Bug 102357 - Remove Node::aboutToUnload and be more explicit about what it was for
Summary: Remove Node::aboutToUnload and be more explicit about what it was for
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-15 02:13 PST by Elliott Sprehn
Modified: 2012-11-15 11:40 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.66 KB, patch)
2012-11-15 02:17 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (5.60 KB, patch)
2012-11-15 02:33 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2012-11-15 03:10 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (5.87 KB, patch)
2012-11-15 09:42 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-11-15 02:13:38 PST
Remove Node::aboutToUnload and be more explicit about what it was for
Comment 1 Elliott Sprehn 2012-11-15 02:17:10 PST
Created attachment 174376 [details]
Patch
Comment 2 Early Warning System Bot 2012-11-15 02:25:37 PST
Comment on attachment 174376 [details]
Patch

Attachment 174376 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14845391
Comment 3 Early Warning System Bot 2012-11-15 02:27:05 PST
Comment on attachment 174376 [details]
Patch

Attachment 174376 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14843514
Comment 4 Elliott Sprehn 2012-11-15 02:33:38 PST
Created attachment 174382 [details]
Patch

Fix silly copy and paste fail
Comment 5 Early Warning System Bot 2012-11-15 02:41:50 PST
Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14831723
Comment 6 Early Warning System Bot 2012-11-15 02:43:18 PST
Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14848301
Comment 7 EFL EWS Bot 2012-11-15 02:58:33 PST
Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14837715
Comment 8 WebKit Review Bot 2012-11-15 03:06:08 PST
Comment on attachment 174382 [details]
Patch

Attachment 174382 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14833672
Comment 9 Elliott Sprehn 2012-11-15 03:10:42 PST
Created attachment 174390 [details]
Patch
Comment 10 Ryosuke Niwa 2012-11-15 03:29:37 PST
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.
Comment 11 Elliott Sprehn 2012-11-15 09:41:21 PST
(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.
Comment 12 Elliott Sprehn 2012-11-15 09:42:51 PST
Created attachment 174469 [details]
Patch for landing
Comment 13 Ryosuke Niwa 2012-11-15 11:31:02 PST
(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...
Comment 14 WebKit Review Bot 2012-11-15 11:40:24 PST
Comment on attachment 174469 [details]
Patch for landing

Clearing flags on attachment: 174469

Committed r134806: <http://trac.webkit.org/changeset/134806>
Comment 15 WebKit Review Bot 2012-11-15 11:40:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Ojan Vafai 2012-11-15 11:40:40 PST
(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.