Bug 109426

Summary: Web Inspector: Simplify SplitView to rely more on CSS
Product: WebKit Reporter: Vladislav Kaznacheev <kaznacheev>
Component: Web Inspector (Deprecated)Assignee: Vladislav Kaznacheev <kaznacheev>
Status: RESOLVED FIXED    
Severity: Normal CC: aandrey, apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch pfeldman: review-

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