Bug 102357 – Remove Node::aboutToUnload and be more explicit about what it was for

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Remove Node::aboutToUnload and be more explicit about what it was for

[Elliott Sprehn]

Created attachment 174376 [details]
Patch

[Early Warning System Bot]

Comment on attachment 174376 [details]
Patch

Attachment 174376 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14845391

[Early Warning System Bot]

Comment on attachment 174376 [details]
Patch

Attachment 174376 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14843514

[Elliott Sprehn]

Created attachment 174382 [details]
Patch

Fix silly copy and paste fail

[Early Warning System Bot]

Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14831723

[Early Warning System Bot]

Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14848301

[EFL EWS Bot]

Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14837715

[WebKit Review Bot]

Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14833672

[Elliott Sprehn]

Created attachment 174390 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 174390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174390&action=review

> Source/WebCore/ChangeLog:14
> +        No new tests, this is just a refactor.

refactoring?

> Source/WebCore/loader/FrameLoader.cpp:395
> +                if (currentFocusedNode && currentFocusedNode->toInputElement())

It's probably better if we had a local variable to avoid calling toInputElement twice.

[Elliott Sprehn]

(In reply to comment #10)
> (From update of attachment 174390 [details])
> ...
> > Source/WebCore/loader/FrameLoader.cpp:395
> > +                if (currentFocusedNode && currentFocusedNode->toInputElement())
> 
> It's probably better if we had a local variable to avoid calling toInputElement twice.

There's nothing performance sensitive here that should require caching the value and that just adds more lines of code.

[Elliott Sprehn]

Created attachment 174469 [details]
Patch for landing

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 174390 [details] [details])
> > ...
> > > Source/WebCore/loader/FrameLoader.cpp:395
> > > +                if (currentFocusedNode && currentFocusedNode->toInputElement())
> > 
> > It's probably better if we had a local variable to avoid calling toInputElement twice.
> 
> There's nothing performance sensitive here that should require caching the value and that just adds more lines of code.

Death by thousand cuts...

[WebKit Review Bot]

Comment on attachment 174469 [details]
Patch for landing

Clearing flags on attachment: 174469

Committed r134806: <http://trac.webkit.org/changeset/134806>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

(In reply to comment #13)
> Death by thousand cuts...

Isn't that an argument for any premature optimization? There's one small cut here that occurs during an already very expensive operation. Death by a thousand cuts applies to small costs that are distributed throughout the codebase and so they wouldn't show up on a profile even though they do affect overall performance. That doesn't seem to be the case here.