Apply :target styling even if fragment scrolling was skipped.

Apply :target styling even if fragment scrolling was skipped.

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

[skobes, 2015-06-08 19:11:03]

Patchset #1 (id:1) has been deleted

[skobes, 2015-06-08 20:53:38]

Patchset #1 (id:20001) has been deleted

[skobes, 2015-06-08 21:44:05]

skobes@chromium.org changed reviewers:
+ japhet@chromium.org

[Nate Chapin, 2015-06-08 21:54:29]

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:244: void maintainScrollPositionAtAnchor(Node*);
Just to make sure I'm reading this correct: the only change to
maintainScrollPositionAtAnchor() is making it private?

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:242: enum UrlFragmentBehavior {
Would this be more readable as...

enum class UrlFragmentBehavior {
    Scroll,
    DontScroll
};

?

https://codereview.chromium.org/1165233002/diff/40001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:190: void processFragment(const KURL&, bool
isSameDocument);
Nit: bool->enum if possible. We have a LoadStartType to differentiate
same/different document navigations, maybe use that?

[skobes, 2015-06-08 22:04:30]

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:244: void maintainScrollPositionAtAnchor(Node*);
On 2015/06/08 21:54:29, Nate Chapin wrote:
> Just to make sure I'm reading this correct: the only change to
> maintainScrollPositionAtAnchor() is making it private?

Yes... I was making scrollToAnchor(const String&) private and noticed that this
one can be private as well.

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:242: enum UrlFragmentBehavior {
On 2015/06/08 21:54:29, Nate Chapin wrote:
> Would this be more readable as...
> 
> enum class UrlFragmentBehavior {
>     Scroll,
>     DontScroll
> };
> 
> ?

Do we want to start using enum class?  None of the other enums in FrameView are
done that way so I was trying to be consistent.

https://codereview.chromium.org/1165233002/diff/40001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.h:190: void processFragment(const KURL&, bool
isSameDocument);
On 2015/06/08 21:54:29, Nate Chapin wrote:
> Nit: bool->enum if possible. We have a LoadStartType to differentiate
> same/different document navigations, maybe use that?

Done.

[Nate Chapin, 2015-06-08 22:17:31]

LGTM

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1165233002/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:242: enum UrlFragmentBehavior {
On 2015/06/08 22:04:30, skobes wrote:
> On 2015/06/08 21:54:29, Nate Chapin wrote:
> > Would this be more readable as...
> > 
> > enum class UrlFragmentBehavior {
> >     Scroll,
> >     DontScroll
> > };
> > 
> > ?
> 
> Do we want to start using enum class?  None of the other enums in FrameView
are
> done that way so I was trying to be consistent.

I've seen enum classes show up in blink lately and had them recommended to me a
couple times. I don't feel strongly; my comment was intended as an honest
question.

[skobes, 2015-06-08 22:18:44]

Thanks.

On 2015/06/08 22:17:31, Nate Chapin wrote:
> I've seen enum classes show up in blink lately and had them recommended to me
a
> couple times. I don't feel strongly; my comment was intended as an honest
> question.

IMO the readability difference is negligible.

[skobes, 2015-06-08 22:18:47]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2015-06-08 22:19:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165233002/60001

[commit-bot: I haz the power, 2015-06-08 23:51:27]

Committed patchset #2 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196714