isActiveFocus() is a more meaningful name

isActiveFocus() is a more meaningful name

Previously the function was named isActive() which
didn't lay emphasis on the behavior and purpose of
the function. There was a FIXME to rename the
function to isActiveFocus().

Patch for doing the same.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172911

[suyash, 2014-04-29 14:40:04]

Please take a look! Thanks :)

[bokan, 2014-04-29 15:05:34]

On 2014/04/29 14:40:04, suyash wrote:
> Please take a look! Thanks :)

Thanks for the cleanup! LGTM (not an OWNER though)

[suyash, 2014-04-29 15:32:20]

On 2014/04/29 15:05:34, bokan wrote:
> On 2014/04/29 14:40:04, suyash wrote:
> > Please take a look! Thanks :)
> 
> Thanks for the cleanup! LGTM (not an OWNER though)

thanks for the review! :)

[pdr., 2014-04-29 16:47:11]

On 2014/04/29 15:32:20, suyash wrote:
> On 2014/04/29 15:05:34, bokan wrote:
> > On 2014/04/29 14:40:04, suyash wrote:
> > > Please take a look! Thanks :)
> > 
> > Thanks for the cleanup! LGTM (not an OWNER though)
> 
> thanks for the review! :)

LGTM

[pdr., 2014-04-29 16:47:15]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:47:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/suyash.s@samsung.com/260353003/1

[suyash, 2014-04-29 18:07:35]

@pdr Thanks for the review :)

[bokan, 2014-04-29 19:49:17]

On 2014/04/29 18:07:35, suyash wrote:
> @pdr Thanks for the review :)

FYI, it looks like you still need OWNER approval for Source/web

[pdr., 2014-04-29 19:49:53]

On 2014/04/29 19:49:17, bokan wrote:
> On 2014/04/29 18:07:35, suyash wrote:
> > @pdr Thanks for the review :)
> 
> FYI, it looks like you still need OWNER approval for Source/web

+cc abarth for Source/web

[abarth-chromium, 2014-04-29 19:54:04]

Source/web rslgtm

[eseidel, 2014-04-29 20:02:01]

What does "isActiveFocus" mean?

I'm not sure this is any more clear to me.

There is a CSS concept of :active
http://www.w3schools.com/cssref/sel_active.asp

Which I believe this previously aligned with.

not lgtm, until this is explained better. :)

[bokan, 2014-04-29 20:26:07]

On 2014/04/29 20:02:01, eseidel wrote:
> What does "isActiveFocus" mean?
> 
> I'm not sure this is any more clear to me.
> 
> There is a CSS concept of :active
> http://www.w3schools.com/cssref/sel_active.asp
> 
> Which I believe this previously aligned with.
> 
> not lgtm, until this is explained better. :)

I'll explain since I added the TODO. I don't think it had anything to do with
CSS :active, all the implementations referenced the Page's FocusController and
whether that was "isActive". Unfortunately it doesn't mention the difference
between isActive and isFocused, but it does seem to be a focus-based notion so I
thought the name was at least an improvement on the existing essentially
meaningless name. Perhaps isFocusControllerActive might be better?

[commit-bot: I haz the power, 2014-04-30 04:06:25]

Change committed as 172911

[suyash, 2014-04-30 05:34:41]

On 2014/04/30 04:06:25, I haz the power (commit-bot) wrote:
> Change committed as 172911

thanks for the review abarth and eric. :)

@bokan thanks for the description. I should've been more comprehensive in my
patch description (pointer for the next time).

So as the patch is landed, is there the consensual need to rename the function
to isFocusControllerActive()? If agreed upon , i can upload another patch
pertaining to it.

To be honest, isFocusControllerActive() does seem to sound and fit in better. :)

[eseidel, 2014-04-30 05:43:32]

I still don't understand what this code is attempting to do.

[bokan, 2014-04-30 14:20:32]

On 2014/04/30 05:43:32, eseidel wrote:
> I still don't understand what this code is attempting to do.

I did a little more digging, it looks like this is a page level concept. A Page
is active if the embedder's window has focus. It looks like this method is only
ever used on Mac; it's used to tell if the scrollbars are in the window with
focus so they can be given a distinctive tint. Perhaps isContainingWindowFocused
or isContainingWindowActive might be better choices? I think that's a lot
clearer.

[jochen (gone - plz use gerrit), 2014-04-30 14:28:44]

On 2014/04/30 14:20:32, bokan wrote:
> On 2014/04/30 05:43:32, eseidel wrote:
> > I still don't understand what this code is attempting to do.
> 
> I did a little more digging, it looks like this is a page level concept. A
Page
> is active if the embedder's window has focus. It looks like this method is
only
> ever used on Mac; it's used to tell if the scrollbars are in the window with
> focus so they can be given a distinctive tint. Perhaps
isContainingWindowFocused
> or isContainingWindowActive might be better choices? I think that's a lot
> clearer.

I think isActive() was a better name. We also use active/activate all over the
place to bring an OS-level window to the front.

[jochen (gone - plz use gerrit), 2014-04-30 14:29:10]

On 2014/04/30 14:28:44, jochen (OOO Wed-Thu) wrote:
> On 2014/04/30 14:20:32, bokan wrote:
> > On 2014/04/30 05:43:32, eseidel wrote:
> > > I still don't understand what this code is attempting to do.
> > 
> > I did a little more digging, it looks like this is a page level concept. A
> Page
> > is active if the embedder's window has focus. It looks like this method is
> only
> > ever used on Mac; it's used to tell if the scrollbars are in the window with
> > focus so they can be given a distinctive tint. Perhaps
> isContainingWindowFocused
> > or isContainingWindowActive might be better choices? I think that's a lot
> > clearer.
> 
> I think isActive() was a better name. We also use active/activate all over the
> place to bring an OS-level window to the front.

You could argue that isActive should be a method on Chrome

[bokan, 2014-04-30 14:42:51]

On 2014/04/30 14:29:10, jochen (OOO Wed-Thu) wrote:
> On 2014/04/30 14:28:44, jochen (OOO Wed-Thu) wrote:
> > On 2014/04/30 14:20:32, bokan wrote:
> > > On 2014/04/30 05:43:32, eseidel wrote:
> > > > I still don't understand what this code is attempting to do.
> > > 
> > > I did a little more digging, it looks like this is a page level concept. A
> > Page
> > > is active if the embedder's window has focus. It looks like this method is
> > only
> > > ever used on Mac; it's used to tell if the scrollbars are in the window
with
> > > focus so they can be given a distinctive tint. Perhaps
> > isContainingWindowFocused
> > > or isContainingWindowActive might be better choices? I think that's a lot
> > > clearer.
> > 
> > I think isActive() was a better name. We also use active/activate all over
the
> > place to bring an OS-level window to the front.
> 
> You could argue that isActive should be a method on Chrome

That feels better, is there a kosher way for Scrollbar (in Source/platform) to
access Chrome (or a related class)?

[bokan, 2014-04-30 15:16:36]

A revert of this CL has been created in
https://codereview.chromium.org/269433002/ by bokan@chromium.org.

The reason for reverting is: Clearly there's opposition to this naming, best to
revert while we discuss..

[eseidel, 2014-04-30 15:50:58]

A path forward:

I don't really care what the name is, so long as you have rendering folks signed
off on it.

I'm sorry for holding up your CL, but I'm not sure this FIXME is worth fixing. 
We might just remove it.

Most FIXMEs in the code require more context to fix that just what the FIXME
says.  In this case I was worried (based on watching other FIXME fix-it attempts
from this week) that this FIXME was just being blindly followed without proper
context.