Avoid calling updateAllLifecyclePhasesExceptPaint in SVGImage if not needed.

Avoid calling updateAllLifecyclePhasesExceptPaint in SVGImage if not needed.

BUG=613662

[chrishtr, 2016-05-20 18:50:40]

Description was changed from

==========
none


none

BUG=
==========

to

==========
Remove call to updateAllLifecyclePhasesExceptPaint in SVGImage.

BUG=613662
==========

[chrishtr, 2016-05-20 22:43:06]

Description was changed from

==========
Remove call to updateAllLifecyclePhasesExceptPaint in SVGImage.

BUG=613662
==========

to

==========
Avoid calling updateAllLifecyclePhasesExceptPaint in SVGImage if not needed.

BUG=613662
==========

[chrishtr, 2016-05-23 20:21:34]

chrishtr@chromium.org changed reviewers:
+ fs@opera.com

[fs, 2016-05-23 21:25:57]

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1465: if (m_currentUrl == url
&& m_currentUrlFragmentBehavior == behavior)
Is the general gain great enough to store the url in FrameView, or could we just
store it in SVGImage for now? (Storing the behavior wouldn't be needed because
it's always the same.) Forwarding the return value probably still makes sense.
Another thing to take into account is that if rendering isn't ready on the first
call, the url would still be stored and then the call to processUrlFragment
after rendering is ready would do nothing.

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1482:
processUrlFragmentHelper(decodeURLEscapeSequences(fragmentIdentifier,
m_frame->document()->encoding()), behavior);
if (...)
  return processUrlFragmentHelp...

return false;

?

(Or reverse the if-condition et.c)

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:279: // Returns false if the
URL behavior is unchanged from last time this method was called.
Maybe also mention that false is returned if the fragment is not applied/no
scroll happens et.c?

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:351:
view->updateAllLifecyclePhasesExceptPaint();
Maybe add some assert too to verify lifecycle assumptions before/after? (At
least after.)

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Widget.cpp (right):

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Widget.cpp:30: #include
"wtf/text/WTFString.h"
Hmm?

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Widget.cpp:77: IntRect
newFrameRect(IntRect(location(), s));
newFrameRect(location(), s);

Could also just compare the size first since the location is trivially
invariant.

[chrishtr, 2016-05-23 21:29:31]

I'm going to drop this CL because of the issue you raised. The impact on the
referenced benchmark was very small anyway.

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1996363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1465: if (m_currentUrl == url
&& m_currentUrlFragmentBehavior == behavior)
On 2016/05/23 at 21:25:56, fs wrote:
> Is the general gain great enough to store the url in FrameView, or could we
just store it in SVGImage for now? (Storing the behavior wouldn't be needed
because it's always the same.) Forwarding the return value probably still makes
sense. Another thing to take into account is that if rendering isn't ready on
the first call, the url would still be stored and then the call to
processUrlFragment after rendering is ready would do nothing.

Indeed. The last point you made is probably a bug in this CL..