Bug 68968 - Only resolve direction aware properties when properties are direction aware.
Summary: Only resolve direction aware properties when properties are direction aware.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-27 21:02 PDT by Luke Macpherson
Modified: 2011-09-28 18:00 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.44 KB, patch)
2011-09-27 21:05 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2011-09-27 23:33 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (2.13 KB, patch)
2011-09-28 16:45 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-09-27 21:02:06 PDT
Only resolve direction aware properties when properties are direction aware.
Comment 1 Luke Macpherson 2011-09-27 21:05:20 PDT
Created attachment 108957 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-09-27 22:28:57 PDT
I'm not entirely sure what this does.  How will we know when we get this wrong?  How do we know that we are using the correct list of "directionally aware" properties?
Comment 3 Luke Macpherson 2011-09-27 23:09:20 PDT
(In reply to comment #2)
> I'm not entirely sure what this does.  How will we know when we get this wrong?  How do we know that we are using the correct list of "directionally aware" properties?

At the moment we try to resolve all property id's which is unnecessary. We can't really miss any because all properties are listed in the switch in applyProperty().
Comment 4 Eric Seidel (no email) 2011-09-27 23:13:29 PDT
Comment on attachment 108957 [details]
Patch

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

This seems reasonable to make directional properties slower at the cost of making all other properties faster.

> Source/WebCore/css/CSSStyleSelector.cpp:3635
> +        return applyProperty(CSSProperty::resolveDirectionAwareProperty(id, m_style->direction(), m_style->writingMode()), value);

Are we sure we're calling this with the right arguments?
Comment 5 Luke Macpherson 2011-09-27 23:19:10 PDT
Comment on attachment 108957 [details]
Patch

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

>> Source/WebCore/css/CSSStyleSelector.cpp:3635
>> +        return applyProperty(CSSProperty::resolveDirectionAwareProperty(id, m_style->direction(), m_style->writingMode()), value);
> 
> Are we sure we're calling this with the right arguments?

Looks ok to me - resolveDirectionAwareProperty just translates the direction aware property id to the appropriate property id for the current context.
I might add an ASSERT_NOT_REACHED() to the default case of that function, just to ensure we can't infinite loop here.
Comment 6 Luke Macpherson 2011-09-27 23:33:09 PDT
Created attachment 108971 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-09-27 23:40:29 PDT
Comment on attachment 108971 [details]
Patch

This is much clearer now, thank you.
Comment 8 WebKit Review Bot 2011-09-28 15:29:34 PDT
Comment on attachment 108971 [details]
Patch

Rejecting attachment 108971 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
sed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/css/CSSStyleSelector.cpp
Hunk #1 FAILED at 2360.
Hunk #2 succeeded at 3596 (offset -2 lines).
Hunk #3 succeeded at 3632 (offset -2 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/css/CSSStyleSelector.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9881821
Comment 9 Luke Macpherson 2011-09-28 16:45:07 PDT
Created attachment 109090 [details]
Patch for landing
Comment 10 WebKit Review Bot 2011-09-28 18:00:20 PDT
Comment on attachment 109090 [details]
Patch for landing

Clearing flags on attachment: 109090

Committed r96281: <http://trac.webkit.org/changeset/96281>
Comment 11 WebKit Review Bot 2011-09-28 18:00:25 PDT
All reviewed patches have been landed.  Closing bug.