Switch CaptivePortalTabHelper to use WebContentsUserData.

Switch CaptivePortalTabHelper to use WebContentsUserData.

BUG=107201
TEST=no visible change


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157613

[Avi (use Gerrit), 2012-09-18 21:25:24]

Please review.

[mmenke, 2012-09-19 16:11:30]

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:489:
tab_contents->web_contents())->IsLoginTab();
nit:  We're in the captive_portal namespace, so no need for the
"captive_portal::" prefix.

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_browsertest.cc:1544:
tab_contents->web_contents())->GetTabReloaderForTest();
nit:  We're in the captive_portal namespace, so no need for the
"captive_portal::" prefix.

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:28: int
captive_portal::CaptivePortalTabHelper::kUserDataKey;
You can remove the captive_portal:: prefix and move the declaration down a
couple lines.

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:253:
captive_portal::CaptivePortalTabHelper::FromWebContents(web_contents);
nit:  We're in the captive_portal namespace, so no need for either of those
"captive_portal::" prefixes.

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:266:
tab_contents->web_contents());
nit:  We're in the captive_portal namespace, so no need for either of those
"captive_portal::" prefixes.

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:104: static int
kUserDataKey;
Per Google style, non-constant static member declarations should go at the
bottom, with other member variables.

[Avi (use Gerrit), 2012-09-19 16:41:27]

Updated.

https://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_po...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

https://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_po...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:28: int
captive_portal::CaptivePortalTabHelper::kUserDataKey;
On 2012/09/19 16:11:30, Matt Menke wrote:
> You can remove the captive_portal:: prefix and move the declaration down a
> couple lines.

OK.

https://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_po...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

https://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_po...
chrome/browser/captive_portal/captive_portal_tab_helper.h:104: static int
kUserDataKey;
On 2012/09/19 16:11:30, Matt Menke wrote:
> Per Google style, non-constant static member declarations should go at the
> bottom, with other member variables.

It is constant. It just can't be marked as such, since it would break under VS.
See https://chromiumcodereview.appspot.com/10868115 for the details.

[mmenke, 2012-09-19 16:52:21]

LGTM, modulo comments

https://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_po...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

https://codereview.chromium.org/10946011/diff/10001/chrome/browser/captive_po...
chrome/browser/captive_portal/captive_portal_tab_helper.h:104: static int
kUserDataKey;
On 2012/09/19 16:41:27, Avi wrote:
> On 2012/09/19 16:11:30, Matt Menke wrote:
> > Per Google style, non-constant static member declarations should go at the
> > bottom, with other member variables.
> 
> It is constant. It just can't be marked as such, since it would break under
VS.
> See https://chromiumcodereview.appspot.com/10868115 for the details.

Ok, but that's just weird.  :(

https://codereview.chromium.org/10946011/diff/5011/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

https://codereview.chromium.org/10946011/diff/5011/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:103: explicit
CaptivePortalTabHelper(content::WebContents* web_contents);
This should go after the static int, per Google style.  I believe it should also
go after the friend declarations, per standard Chrome style (Guide is ambiguous
here).

https://codereview.chromium.org/10946011/diff/5011/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.h:104: static int
kUserDataKey;
I believe this should go below the friend declarations, though the style guide
is ambiguous on that.

[Avi (use Gerrit), 2012-09-19 19:21:17]

I cluster those three:

constructor
userdatakey
friend wcud

as they're the three pieces that turn this into a thing that attaches to
WebContents. I'll argue that the style guide is talking about unrelated things.
If there were a typedef that were only used by a single function or member
variable, wouldn't it go right there?

[shinobisms, 2012-09-19 19:23:29]

lgtm

[Avi (use Gerrit), 2012-09-19 19:28:41]

If you insist I'll switch it, but I'd rather cluster things conceptually than
literally follow the style guide, especially if it's ambiguous.

shinobisms, thanks for the LG, but who are you?

[mmenke, 2012-09-19 19:29:19]

On 2012/09/19 19:21:17, Avi wrote:
> I cluster those three:
> 
> constructor
> userdatakey
> friend wcud
> 
> as they're the three pieces that turn this into a thing that attaches to
> WebContents. I'll argue that the style guide is talking about unrelated
things.
> If there were a typedef that were only used by a single function or member
> variable, wouldn't it go right there?

Couldn't you cluster them, and follow the style guide?

 private:
  friend class CaptivePortalBrowserTest;
  friend class CaptivePortalTabHelperTest;

  friend class WebContentsUserData<CaptivePortalTabHelper>;
  static int kUserDataKey;
  explicit CaptivePortalTabHelper(content::WebContents* web_contents);

The case with a single typedef is a little different, since the typedef is
generally used in the function's prototype, which makes things a bit clearer.  I
also think hiding a function with the friend declarations makes it easy to
overlook.  I'd actually prefer a space before the prototype, but can live
without it.

[Avi (use Gerrit), 2012-09-19 19:33:09]

Done.

[commit-bot: I haz the power, 2012-09-19 19:33:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10946011/13002

[commit-bot: I haz the power, 2012-09-19 19:33:13]

Failed to apply patch for chrome/browser/ui/tab_contents/tab_contents.h:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/tab_contents/tab_contents.h
Hunk #1 FAILED at 70.
Hunk #2 succeeded at 188 (offset -7 lines).
Hunk #3 succeeded at 264 (offset -13 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/tab_contents/tab_contents.h.rej

[shinobisms, 2012-09-19 19:33:25]

On 2012/09/19 19:23:29, shinobisms wrote:
> lgtm

[commit-bot: I haz the power, 2012-09-19 19:41:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10946011/14003

[commit-bot: I haz the power, 2012-09-19 19:41:22]

Presubmit check for 10946011-14003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.0s to calculate.

[Ben Goodger (Google), 2012-09-19 19:45:08]

lgtm

[commit-bot: I haz the power, 2012-09-19 19:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10946011/14003

[commit-bot: I haz the power, 2012-09-19 21:36:25]

Change committed as 157613