Bug 16385 - Cleanup kjs_window
Summary: Cleanup kjs_window
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
: 16386 (view as bug list)
Depends on:
Reported: 2007-12-10 13:52 PST by Sam Weinig
Modified: 2007-12-16 20:52 PST (History)
0 users

See Also:

First round of cleanup (48.33 KB, patch)
2007-12-10 14:13 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff
move PausedTimeouts (145.58 KB, patch)
2007-12-16 13:40 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff
move ScheduledAction (146.77 KB, patch)
2007-12-16 16:48 PST, Sam Weinig
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-12-10 13:52:12 PST
Most of the content of kjs_window can be move to more appropriate places (to DOMWindow, WindowFeatures, etc.)
Comment 1 Sam Weinig 2007-12-10 13:54:13 PST
*** Bug 16386 has been marked as a duplicate of this bug. ***
Comment 2 Sam Weinig 2007-12-10 14:13:14 PST
Created attachment 17826 [details]
First round of cleanup
Comment 3 Darin Adler 2007-12-10 14:20:56 PST
Comment on attachment 17826 [details]
First round of cleanup

Please run the sort-Xcode-project-file script on the project file.

+    // - dialogHide: trusted && WindowFeatures::boolFeature(features, "dialoghide"), makes dialog hide when you print
+    // - help: WindowFeatures::boolFeature(features, "help", true), makes help icon appear in dialog (what does it do on Windows?)
+    // - unadorned: trusted && WindowFeatures::boolFeature(features, "unadorned");

Seems a little strange here. Global replace problem?

+static bool isSeparator(::UChar c)

I don't think you need the :: here. It was needed because of "using namespace KJS".

+    {}

We usually put those braces on separate lines.

Where's the update to the .pro and .bkl files?

Otherwise looks fine.
Comment 4 Sam Weinig 2007-12-16 13:40:50 PST
Created attachment 17937 [details]
move PausedTimeouts
Comment 5 Sam Weinig 2007-12-16 14:52:35 PST
The first patch was landed in r28592.
Comment 6 Darin Adler 2007-12-16 15:36:30 PST
Comment on attachment 17937 [details]
move PausedTimeouts

Comment 7 Sam Weinig 2007-12-16 16:30:41 PST
Second patch landed in r28779.
Comment 8 Sam Weinig 2007-12-16 16:48:11 PST
Created attachment 17943 [details]
move ScheduledAction
Comment 9 Maciej Stachowiak 2007-12-16 19:26:52 PST
Comment on attachment 17943 [details]
move ScheduledAction

Please try to use separate bugs for separate patches. Otherwise the state of the bug gets confusing. 

Comment 10 Sam Weinig 2007-12-16 20:52:10 PST
Third patch landed in r28794.  Closing this bug, but will follow up with other bugs for more specific single tasks.