Make WebContentsObserver::web_contents() public

Make WebContentsObserver::web_contents() public

WebContentsObserver::web_contents() is now public + cleanup of unnecessary getters.

BUG=408560
Committed: https://crrev.com/c25f7fb98990a08aa235d1cee10da982b9f4aecc
Cr-Commit-Position: refs/heads/master@{#293274}

[lucinka.brozkova, 2014-08-30 18:36:42]

lucinka.brozkova@gmail.com changed reviewers:
+ asanka@chromium.org, fsamuel@chromium.org, isherman@chromium.org,
jam@chromium.org, jyasskin@chromium.org, mmenke@chromium.org,
pkasting@chromium.org, sadrul@chromium.org, toyoshim@chromium.org,
vsevik@chromium.org

[lucinka.brozkova, 2014-08-30 18:36:43]

Hi jam@,
Thanks for giving the green light in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/f4CLGliOyW0/9eli7....
Please review content/public/browser/web_contents_observer.h.

Dear reviewers named below, please review the files mentioned to your names. The
changes are just simple consequences of making
WebContentsObserver::web_contents() public.
@vsevik:
  chrome/browser/devtools/*
@asanka:
  chrome/browser/download/download_request_limiter.h
  content/browser/download/*
@jyasskin:
  chrome/browser/extensions/*
  extensions/*
@fsamuel:
  chrome/browser/guest_view/*
@mmenke:
  chrome/browser/prerender/prerender_manager.*
@pkasting:
  chrome/browser/infobars/infobar_service.h
  chrome/browser/ui/*
@toyoshim:
  chrome/browser/translate/chrome_translate_client.cc
@isherman:
  components/autofill/*
@sadrul:
  ui/views/controls/webview/webview.h

Thank you.

Lucie

[Jeffrey Yasskin, 2014-08-30 20:05:58]

lgtm for extensions. Note that you could probably have split the cleanup into
separate changes.

[dgozman, 2014-08-31 08:25:28]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2014-08-31 08:25:29]

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:32: class ObserverWithAccessor :
public content::WebContentsObserver {
The whole purpose of this class was to expose web_contents(). You can safely
remove it and replace with content::WebContentsOberver.

[lucinka.brozkova, 2014-08-31 16:13:54]

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:32: class ObserverWithAccessor :
public content::WebContentsObserver {
There are two options:

First:
Keeping this class, because it is allowing construction and destruction of
content::WebContentsObserver

Second:
Remove this class and make constructor
content::WebContentsObserver(content::WebContents* web_contents) and destructor
public.

I favour the second option. Before, creating just WebContentsObserver was not
useful, but now it can provide a safe pointer to WebContents. Safe in the sense
that it is NULL if WebContents was destroyed. 

jam@ : What is your opinion about this?

On 2014/08/31 08:25:29, dgozman wrote:
> The whole purpose of this class was to expose web_contents(). You can safely
> remove it and replace with content::WebContentsOberver.

[Takashi Toyoshima, 2014-09-01 13:57:53]

lgtm on translate

[asanka, 2014-09-02 14:14:27]

lgtm for /download/

[sadrul, 2014-09-02 14:16:35]

ui/views/ lgtm

[mmenke, 2014-09-02 14:22:42]

On 2014/09/02 14:16:35, sadrul wrote:
> ui/views/ lgtm

browser/prerender/ LGTM

[Fady Samuel, 2014-09-02 14:24:23]

guest_view lgtm

[Peter Kasting, 2014-09-02 19:37:07]

LGTM

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:91: ->Submit(search_terms);
Nit: Don't change wrapping here

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:35: : false;
Nit: Shorter:

  return web_contents() &&
      SearchTabHelper::FromWebContents(web_contents())->SupportsInstant();

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:44: web_contents()->GetURL() ==
GURL(chrome::kChromeSearchLocalNtpUrl);
Nit: Don't change indenting here

[jam, 2014-09-02 19:43:58]

lgtm, thank you

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:32: class ObserverWithAccessor :
public content::WebContentsObserver {
On 2014/08/31 16:13:54, lucinka.brozkova wrote:
> There are two options:
> 
> First:
> Keeping this class, because it is allowing construction and destruction of
> content::WebContentsObserver
> 
> Second:
> Remove this class and make constructor
> content::WebContentsObserver(content::WebContents* web_contents) and
destructor
> public.
> 
> I favour the second option. Before, creating just WebContentsObserver was not
> useful, but now it can provide a safe pointer to WebContents. Safe in the
sense
> that it is NULL if WebContents was destroyed. 
> 
> jam@ : What is your opinion about this?
> 
> On 2014/08/31 08:25:29, dgozman wrote:
> > The whole purpose of this class was to expose web_contents(). You can safely
> > remove it and replace with content::WebContentsOberver.
> 

I don't think we should use a WebContentsObserver as a safe poiner to WCO. If we
really did want to do such a thing, which isn't clear to me, we would make
WebContents vend weak pointers.

[Ilya Sherman, 2014-09-02 20:25:26]

//components/autofill/* LGTM.

[lucinka.brozkova, 2014-09-03 08:16:25]

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.h (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.h:32: class ObserverWithAccessor :
public content::WebContentsObserver {
On 2014/09/02 19:43:58, jam wrote:
> On 2014/08/31 16:13:54, lucinka.brozkova wrote:
> > There are two options:
> > 
> > First:
> > Keeping this class, because it is allowing construction and destruction of
> > content::WebContentsObserver
> > 
> > Second:
> > Remove this class and make constructor
> > content::WebContentsObserver(content::WebContents* web_contents) and
> destructor
> > public.
> > 
> > I favour the second option. Before, creating just WebContentsObserver was
not
> > useful, but now it can provide a safe pointer to WebContents. Safe in the
> sense
> > that it is NULL if WebContents was destroyed. 
> > 
> > jam@ : What is your opinion about this?
> > 
> > On 2014/08/31 08:25:29, dgozman wrote:
> > > The whole purpose of this class was to expose web_contents(). You can
safely
> > > remove it and replace with content::WebContentsOberver.
> > 
> 
> I don't think we should use a WebContentsObserver as a safe poiner to WCO. If
we
> really did want to do such a thing, which isn't clear to me, we would make
> WebContents vend weak pointers.

Acknowledged.

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:91: ->Submit(search_terms);
On 2014/09/02 19:37:06, Peter Kasting wrote:
> Nit: Don't change wrapping here

Done.

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:35: : false;
On 2014/09/02 19:37:07, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return web_contents() &&
>       SearchTabHelper::FromWebContents(web_contents())->SupportsInstant();

Done.

https://codereview.chromium.org/525793002/diff/60001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_page.cc:44: web_contents()->GetURL() ==
GURL(chrome::kChromeSearchLocalNtpUrl);
On 2014/09/02 19:37:07, Peter Kasting wrote:
> Nit: Don't change indenting here

Done.

[lucinka.brozkova, 2014-09-03 08:30:13]

The CQ bit was checked by lucinka.brozkova@gmail.com

[commit-bot: I haz the power, 2014-09-03 08:31:10]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/lucinka.brozkova@gmail.com/525793002/8...

[commit-bot: I haz the power, 2014-09-03 09:33:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-03 10:52:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-03 10:53:00]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lucinka.brozkova, 2014-09-04 06:01:59]

The CQ bit was checked by lucinka.brozkova@gmail.com

[commit-bot: I haz the power, 2014-09-04 06:02:54]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/lucinka.brozkova@gmail.com/525793002/8...

[commit-bot: I haz the power, 2014-09-04 06:45:03]

Committed patchset #4 (id:80001) as 09440c9d77239696e0772d6d891dc8d826472fc7

[commit-bot: I haz the power, 2014-09-10 03:30:26]

Patchset 4 (id:??) landed as
https://crrev.com/c25f7fb98990a08aa235d1cee10da982b9f4aecc
Cr-Commit-Position: refs/heads/master@{#293274}