Bug 90913 - Regression(121518) TextFieldDecorationElement formatting is broken
Summary: Regression(121518) TextFieldDecorationElement formatting is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 59816
Blocks: 72352
  Show dependency treegraph
 
Reported: 2012-07-10 14:51 PDT by Garrett Casto
Modified: 2012-08-15 20:06 PDT (History)
6 users (show)

See Also:


Attachments
Linux UI (58.91 KB, image/png)
2012-07-23 22:46 PDT, Garrett Casto
no flags Details
Patch (9.55 KB, patch)
2012-08-01 01:02 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Updated to ToT (9.58 KB, patch)
2012-08-15 03:52 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Perf chart (15.98 KB, text/html)
2012-08-15 03:55 PDT, Hajime Morrita
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Garrett Casto 2012-07-10 14:51:41 PDT
Before this change, a TextFieldDecorationElement was rendered on the right hand side text field. After it, it is rendered on the left side of the text field.
Comment 1 Hajime Morrita 2012-07-10 21:57:27 PDT
Hmm. How can I try that?
Comment 2 Garrett Casto 2012-07-11 10:23:13 PDT
Ah, sorry for not giving a way to reproduce. The only thing that I know of that uses this is a feature for generating passwords that I'm working on.

1) Add --enable-password-generation to the command line
2) Make sure that you are signed in with sync enabled for passwords.
3) Go to https://myaccount.nytimes.com/register

There should be a small key icon in the first password field on the left side of the field. It should be on the right side. It's possible that there is an easier way to test this, but I'm not sure what it is.
Comment 3 Hajime Morrita 2012-07-22 22:17:43 PDT
I tried this on Chromium Linux ToT but I couldn't see any such icons. 
Am I doing something wrong?
Comment 4 Garrett Casto 2012-07-23 22:46:03 PDT
Created attachment 153964 [details]
Linux UI
Comment 5 Garrett Casto 2012-07-23 23:00:59 PDT
One other reason it would not show up is if you've every clicked on the "never remember passwords for this site" infobar for nytimes.com. To be safe, I'd just trying using a clean profile. I've attached an image of how the UI should look in case it's not obvious when it shows up.
Comment 6 Hajime Morrita 2012-07-24 00:54:44 PDT
Oops. I accidentally used older binary. Now it shows the key icon.
Thanks.
Comment 7 Hajime Morrita 2012-07-26 17:57:31 PDT
Created a reduction: http://jsfiddle.net/
Comment 8 Hajime Morrita 2012-07-26 17:57:47 PDT
(In reply to comment #7)
> Created a reduction: http://jsfiddle.net/
Oops: http://jsfiddle.net/sggwj/
Comment 9 Hajime Morrita 2012-08-01 01:02:46 PDT
Created attachment 155739 [details]
Patch
Comment 10 Hajime Morrita 2012-08-01 01:04:13 PDT
Hi Dimitri, could you take a look?
This is happened to be a long awaited fix for me and Hayato-san :-)
Comment 11 Dimitri Glazkov (Google) 2012-08-01 09:34:59 PDT
Comment on attachment 155739 [details]
Patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:95
> +    ComposedShadowTreeWalker walker(m_node);

This area is super-hot. What are the performance implications of this change?
Comment 12 Hajime Morrita 2012-08-02 00:47:50 PDT
(In reply to comment #11)
> (From update of attachment 155739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155739&action=review
> 
> > Source/WebCore/dom/NodeRenderingContext.cpp:95
> > +    ComposedShadowTreeWalker walker(m_node);
> 
> This area is super-hot. What are the performance implications of this change?
True. I observed certain regression on Dromaeo. 
I'm going to return here with ComposedShadowTreeWalker speedup.
Comment 13 Hajime Morrita 2012-08-08 02:28:20 PDT
I want Bug 59816 to make it faster. IsInShadow flag will help up a lot.
Comment 14 Eric Seidel (no email) 2012-08-12 03:37:08 PDT
Could you post the perf numbers to the bug?  Would be good to document how bad this is for perf.
Comment 15 Hajime Morrita 2012-08-15 03:52:37 PDT
Created attachment 158540 [details]
Updated to ToT
Comment 16 Hajime Morrita 2012-08-15 03:55:49 PDT
Created attachment 158541 [details]
Perf chart

Looks like I was wrong.
Attached chart shows no significant slowdown and show even slight speedup.
Now personally I'm rather optimistic
and would like to land this to see how it affects in Page cycler speed.
Comment 17 Hajime Morrita 2012-08-15 03:57:45 PDT
(In reply to comment #16)
> Created an attachment (id=158541) [details]
> Perf chart
First two are patch applied.
Last two are original.
Comment 18 Dimitri Glazkov (Google) 2012-08-15 09:40:15 PDT
Comment on attachment 158540 [details]
Updated to ToT

ok. Not flipping the cq+, because I would prefer if you land it and watch it :)
Comment 19 Hajime Morrita 2012-08-15 19:24:26 PDT
Comment on attachment 158540 [details]
Updated to ToT

Fair enough. Let me flip it :-)
Comment 20 WebKit Review Bot 2012-08-15 20:06:15 PDT
Comment on attachment 158540 [details]
Updated to ToT

Clearing flags on attachment: 158540

Committed r125739: <http://trac.webkit.org/changeset/125739>
Comment 21 WebKit Review Bot 2012-08-15 20:06:20 PDT
All reviewed patches have been landed.  Closing bug.