Count usage of #svgView(...) and plain SVG <view> targets

Count usage of #svgView(...) and plain SVG <view> targets

BUG=562099
Committed: https://crrev.com/160390068163da225476f5fede45323b77cd6f21
Cr-Commit-Position: refs/heads/master@{#361993}

[davve, 2015-11-26 13:20:07]

davve@opera.com changed reviewers:
+ fs@opera.com, philipj@opera.com

[davve, 2015-11-26 13:20:08]

PTAL

[fs, 2015-11-26 13:30:24]

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:716:
UseCounter::count(document(), UseCounter::SVGSVGElementFragment);
IIRC, this method still gets called when there's no fragment, so I think this
should be within "if (!name.isEmpty())" or so?

[davve, 2015-11-26 13:35:37]

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:716:
UseCounter::count(document(), UseCounter::SVGSVGElementFragment);
On 2015/11/26 13:30:23, fs wrote:
> IIRC, this method still gets called when there's no fragment, so I think this
> should be within "if (!name.isEmpty())" or so?

I only found one call-site, FrameView::processUrlFragmentHelper, with one call
site, FrameView::processUrlFragment that checks for fragment.

Adding it seems like the safer option though.

[fs, 2015-11-26 13:38:36]

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:716:
UseCounter::count(document(), UseCounter::SVGSVGElementFragment);
On 2015/11/26 at 13:35:37, David Vest wrote:
> On 2015/11/26 13:30:23, fs wrote:
> > IIRC, this method still gets called when there's no fragment, so I think
this
> > should be within "if (!name.isEmpty())" or so?
> 
> I only found one call-site, FrameView::processUrlFragmentHelper, with one call
site, FrameView::processUrlFragment that checks for fragment.
> 
> Adding it seems like the safer option though.

Yes, in processUrlFragment we have:

    // If our URL has no ref, then we have no place we need to jump to.
    // OTOH If CSS target was set previously...
...
    // Similarly for svg, if we had a previous svgView() then we need to reset
    // the initial view if we don't have a fragment.
    if (!url.hasFragmentIdentifier() && !m_frame->document()->cssTarget() &&
!m_frame->document()->isSVGDocument())
        return false;

[davve, 2015-11-26 13:43:20]

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/1471963007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:716:
UseCounter::count(document(), UseCounter::SVGSVGElementFragment);
On 2015/11/26 13:38:36, fs wrote:
> On 2015/11/26 at 13:35:37, David Vest wrote:
> > On 2015/11/26 13:30:23, fs wrote:
> > > IIRC, this method still gets called when there's no fragment, so I think
> this
> > > should be within "if (!name.isEmpty())" or so?
> > 
> > I only found one call-site, FrameView::processUrlFragmentHelper, with one
call
> site, FrameView::processUrlFragment that checks for fragment.
> > 
> > Adding it seems like the safer option though.
> 
> Yes, in processUrlFragment we have:
> 
>     // If our URL has no ref, then we have no place we need to jump to.
>     // OTOH If CSS target was set previously...
> ...
>     // Similarly for svg, if we had a previous svgView() then we need to reset
>     // the initial view if we don't have a fragment.
>     if (!url.hasFragmentIdentifier() && !m_frame->document()->cssTarget() &&
> !m_frame->document()->isSVGDocument())
>         return false;

Ouch, a !m_frame->document()->isSVGDocument() there. Thanks. Might as well add
that to the test.

[davve, 2015-11-26 14:16:05]

Now limited actual m_useCurrentView = true code paths over two counters.

[davve, 2015-11-26 14:16:25]

Description was changed from

==========
Count usage of #svgView(...) and plain targets

BUG=562099
==========

to

==========
Count usage of #svgView(...) and plain SVG <view> targets

BUG=562099
==========

[fs, 2015-11-26 14:19:36]

On 2015/11/26 at 14:16:05, davve wrote:
> Now limited actual m_useCurrentView = true code paths over two counters.

Ok, so we're only counting the "odder" cases =) (except xpointer(... =D). LGTM.

[davve, 2015-11-26 14:20:52]

davve@opera.com changed reviewers:
+ isherman@chromium.org

[philipj_slow, 2015-11-26 14:27:14]

lgtm

[Ilya Sherman, 2015-11-26 20:51:46]

histograms.xml lgtm

[davve, 2015-11-27 09:22:26]

The CQ bit was checked by davve@opera.com

[davve, 2015-11-27 09:22:27]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
isherman@chromium.org, fs@opera.com
Link to the patchset: https://codereview.chromium.org/1471963007/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-27 09:22:41]

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

[commit-bot: I haz the power, 2015-11-27 10:52:44]

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

[commit-bot: I haz the power, 2015-11-27 10:52:45]

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

[davve, 2015-11-27 11:32:43]

The CQ bit was checked by davve@opera.com

[davve, 2015-11-27 11:32:44]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
isherman@chromium.org, fs@opera.com
Link to the patchset: https://codereview.chromium.org/1471963007/#ps80001
(title: "Add missing expected file")

[commit-bot: I haz the power, 2015-11-27 11:33:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1471963007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1471963007/80001

[commit-bot: I haz the power, 2015-11-27 13:21:24]

Description was changed from

==========
Count usage of #svgView(...) and plain SVG <view> targets

BUG=562099
==========

to

==========
Count usage of #svgView(...) and plain SVG <view> targets

BUG=562099
==========

[commit-bot: I haz the power, 2015-11-27 13:21:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-27 13:22:26]

Description was changed from

==========
Count usage of #svgView(...) and plain SVG <view> targets

BUG=562099
==========

to

==========
Count usage of #svgView(...) and plain SVG <view> targets

BUG=562099

Committed: https://crrev.com/160390068163da225476f5fede45323b77cd6f21
Cr-Commit-Position: refs/heads/master@{#361993}
==========

[commit-bot: I haz the power, 2015-11-27 13:22:27]

Patchset 5 (id:??) landed as
https://crrev.com/160390068163da225476f5fede45323b77cd6f21
Cr-Commit-Position: refs/heads/master@{#361993}