Bug 109426 - Web Inspector: Simplify SplitView to rely more on CSS
Summary: Web Inspector: Simplify SplitView to rely more on CSS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vladislav Kaznacheev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-11 06:51 PST by Vladislav Kaznacheev
Modified: 2013-02-14 08:16 PST (History)
10 users (show)

See Also:

Attachments
Patch (16.34 KB, patch)
2013-02-11 06:54 PST, Vladislav Kaznacheev 		no flags Details | Formatted Diff | Diff
Patch (15.40 KB, patch)
2013-02-12 05:34 PST, Vladislav Kaznacheev 		no flags Details | Formatted Diff | Diff
Patch (15.43 KB, patch)
2013-02-13 04:14 PST, Vladislav Kaznacheev 		no flags Details | Formatted Diff | Diff
Patch (4.85 KB, patch)
2013-02-14 07:52 PST, Vladislav Kaznacheev 		pfeldman: review-
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Vladislav Kaznacheev 2013-02-11 06:51:11 PST 
Currently WebInspector.SplitView code performs quite a few element.style manipulations. Most of these can replaced with simple CSS rules.
Comment 1 Vladislav Kaznacheev 2013-02-11 06:54:35 PST 
Created attachment 187561 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2013-02-11 07:05:04 PST 
Comment on attachment 187561 [details]
Patch

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

Not really familiar with the code, but there's one style nit there.

> Source/WebCore/inspector/front-end/SplitView.js:181
> +        this._firstElement.style.cssText = '';

Web Inspector uses double quotes (") for enclosing string literals.
Comment 3 Vsevolod Vlasov 2013-02-12 05:14:12 PST 
Comment on attachment 187561 [details]
Patch

I would revert the change in SplitView._removeAllLayoutProperties (what if split view elements have styles set externally?), other than that looks good.
Comment 4 Vladislav Kaznacheev 2013-02-12 05:34:04 PST 
Comment on attachment 187561 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SplitView.js:181
>> +        this._firstElement.style.cssText = '';
> 
> Web Inspector uses double quotes (") for enclosing string literals.

Vsevik convinced me to roll back the change to this function, so the problem is no longer there.
Comment 5 Vladislav Kaznacheev 2013-02-12 05:34:55 PST 
Created attachment 187842 [details]
Patch
Comment 6 WebKit Review Bot 2013-02-12 05:52:19 PST 
Comment on attachment 187842 [details]
Patch

Rejecting attachment 187842 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'validate-changelog', '--non-interactive', 187842, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16519216
Comment 7 Vladislav Kaznacheev 2013-02-13 04:14:37 PST 
Created attachment 188051 [details]
Patch
Comment 8 WebKit Review Bot 2013-02-13 05:17:04 PST 
Comment on attachment 188051 [details]
Patch

Clearing flags on attachment: 188051

Committed r142738: <http://trac.webkit.org/changeset/142738>
Comment 9 WebKit Review Bot 2013-02-13 05:17:09 PST 
All reviewed patches have been landed.  Closing bug.
Comment 10 Andrey Adaikin 2013-02-14 07:29:02 PST 
This patch broke SplitView in Canvas profiler. I requested a rollout, but sheriffbot failed.

@Vladislav: Would you manually roll out this patch please, or plz fix the problem. A SplitView in CanvasProfileView.js no longer works.
Comment 11 Vladislav Kaznacheev 2013-02-14 07:52:57 PST 
Created attachment 188351 [details]
Patch
Comment 12 Pavel Feldman 2013-02-14 07:53:52 PST 
Comment on attachment 188351 [details]
Patch

There is a bug-per patch policy that you should follow.
Comment 13 Andrey Adaikin 2013-02-14 08:16:33 PST 
Regression addressed at https://bugs.webkit.org/show_bug.cgi?id=109835