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

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