Use scoped_ptr to document ownership in InstantUnloadHandler.

Use scoped_ptr to document ownership in InstantUnloadHandler.

And some cosmetic changes in other Instant files.

BUG=none
R=jered@chromium.org,sky@chromium.org
TEST=none; no change in functionality.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182397

[sreeram, 2013-02-13 22:56:44]

jered for review
sky for OWNERS

[Jered, 2013-02-13 23:25:48]

https://codereview.chromium.org/12256029/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_unload_handler.cc (left):

https://codereview.chromium.org/12256029/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_unload_handler.cc:81:
contents->GetRenderViewHost()->FirePageBeforeUnload(false);
What does this now?

https://codereview.chromium.org/12256029/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12256029/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:174: content::WebContents*
old_contents =
I think this should be a scoped_ptr and the call should say old_contents.Pass(),
no?

[sreeram, 2013-02-13 23:41:27]

https://codereview.chromium.org/12256029/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_unload_handler.cc (left):

https://codereview.chromium.org/12256029/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_unload_handler.cc:81:
contents->GetRenderViewHost()->FirePageBeforeUnload(false);
On 2013/02/13 23:25:48, Jered wrote:
> What does this now?

The delegate. See line 25 of the new code. I can't do it here after creating the
delegate, since the delegate now owns the WebContents (and ours is NULL).

https://codereview.chromium.org/12256029/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12256029/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:174: content::WebContents*
old_contents =
On 2013/02/13 23:25:48, Jered wrote:
> I think this should be a scoped_ptr and the call should say
old_contents.Pass(),
> no?

Done. Added a comment for clarity too.

[sreeram, 2013-02-13 23:53:11]

https://codereview.chromium.org/12256029/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12256029/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:174: content::WebContents*
old_contents =
On 2013/02/13 23:41:27, sreeram wrote:
> On 2013/02/13 23:25:48, Jered wrote:
> > I think this should be a scoped_ptr and the call should say
> old_contents.Pass(),
> > no?
> 
> Done. Added a comment for clarity too.

Patchset #3 does this correctly now, without any comments (which is the whole
point of using scoped_ptr - to avoid clumsy ownership comments). Thanks!

[Jered, 2013-02-13 23:56:10]

lgtm

https://codereview.chromium.org/12256029/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_unload_handler.cc (left):

https://codereview.chromium.org/12256029/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_unload_handler.cc:81:
contents->GetRenderViewHost()->FirePageBeforeUnload(false);
On 2013/02/13 23:41:27, sreeram wrote:
> On 2013/02/13 23:25:48, Jered wrote:
> > What does this now?
> 
> The delegate. See line 25 of the new code. I can't do it here after creating
the
> delegate, since the delegate now owns the WebContents (and ours is NULL).

Oh right I see it now.

[sreeram, 2013-02-14 00:27:35]

+sky for OWNERS

[sky, 2013-02-14 01:26:10]

LGTM

[commit-bot: I haz the power, 2013-02-14 01:29:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/12256029/8001

[commit-bot: I haz the power, 2013-02-14 01:51:24]

Retried try job too often on linux_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-02-14 01:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/12256029/8001

[commit-bot: I haz the power, 2013-02-14 02:18:57]

Retried try job too often on linux_chromeos for step(s) aura_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, check_deps,
chromeos_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, dbus_unittests, device_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-14 02:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/12256029/8001

[commit-bot: I haz the power, 2013-02-14 06:14:22]

Change committed as 182397