Bug 50949

Created attachment 88270 [details] Patch

Attachment 88270 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1 Source/WebCore/rendering/style/RenderStyle.h:259: _unicodeBidi is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 88270 [details] Patch Canceling the review as there were two more changes to make: 1 - check for line breaks and bail in determineParagraphDirection, since in the first layout we don't have an end iterator 2 - properly set unicode-bidi to plaintext for pre and textarea. New patch coming shortly.

Created attachment 88417 [details] Patch

Attachment 88417 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1 Source/WebCore/rendering/style/RenderStyle.h:259: _unicodeBidi is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

I'd love a review if anyone gets a chance :)

Comment on attachment 88417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88417&action=review I'm concerned this is not the only place you want this code. Otherwise seems fine. > Source/WebCore/html/HTMLElement.cpp:170 > + int bidiAttribute; > + if (hasLocalName(bdoTag)) > + bidiAttribute = CSSValueBidiOverride; > + else if (hasLocalName(preTag) || hasLocalName(textareaTag)) > + bidiAttribute = CSSValueWebkitPlaintext; > + else > + bidiAttribute = CSSValueEmbed; > + addCSSProperty(attr, CSSPropertyUnicodeBidi, bidiAttribute); This is a super-weird place for this code! I guess since I we have eto leave his here we should abstract this into a unicodeBidFromDirAuto() helper. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:87 > +static void determineParagraphDirection(Direction& dir, InlineIterator iter) > +{ So this is basically "first strong direction in paragarph"? I'm not sure where this logic goes. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1266 > + if (style()->unicodeBidi() == Plaintext) > + determineParagraphDirection(direction, InlineIterator(this, bidiFirst(this, 0), pos)); I think we're going to want to start passing unicodeBidi() to BidiContexts :( Are we sure this is the only place this needs to be done? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1270 > resolver.setLastStrongDir(direction); > resolver.setLastDir(direction); > resolver.setEorDir(direction); > - resolver.setContext(BidiContext::create(ltr ? 0 : 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM)); > + resolver.setContext(BidiContext::create(direction == LeftToRight ? 0 : 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM)); I think I have an outstanding conflicting patch here.

Comment on attachment 88417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88417&action=review >> Source/WebCore/html/HTMLElement.cpp:170 >> + addCSSProperty(attr, CSSPropertyUnicodeBidi, bidiAttribute); > > This is a super-weird place for this code! I guess since I we have eto leave his here we should abstract this into a unicodeBidFromDirAuto() helper. That makes sense. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:87 >> +{ > > So this is basically "first strong direction in paragarph"? I'm not sure where this logic goes. That's correct. First the first character with a strong directionality is what determines the paragraph's base writing direction. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1266 >> + determineParagraphDirection(direction, InlineIterator(this, bidiFirst(this, 0), pos)); > > I think we're going to want to start passing unicodeBidi() to BidiContexts :( > > Are we sure this is the only place this needs to be done? These two cases cover it: when we initially set up the BidiContext for the block, and again after each paragraph separator. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1270 >> + resolver.setContext(BidiContext::create(direction == LeftToRight ? 0 : 1, direction, style()->unicodeBidi() == Override, FromStyleOrDOM)); > > I think I have an outstanding conflicting patch here. Link? If it's going to be a blocking issue list it as such so I know when to update this patch.

Created attachment 91362 [details] Patch

Attachment 91362 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/RenderStyle.h:259: _unicodeBidi is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

@Eric: Following up wondering about the conflicting patch you had on this. When does it land? Could you add it as a blocker so I can track it? I think I cleared up your other issue and appreciate your feedback :)

Created attachment 98088 [details] Patch

Attachment 98088 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/RenderStyle.h:259: _unicodeBidi is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98088&action=review I think I need to look at this again later. > Source/WebCore/html/HTMLElement.cpp:129 > +static int unicodeBidiAttributeForDirAuto(HTMLElement* element) Should this be marked inline? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:115 > +static void determineParagraphDirection(TextDirection& dir, InlineIterator iter) Should InlineIterator be passed as a reference? do you actually want it to be copied here? Maybe that would be more clear with a const & which you explicitly copy? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:967 > + bool isNewParagraph = lineInfo.previousLineBrokeCleanly(); is this true? that it's a "new paragraph"? You might want to say isNewUBAParagrah? I'm not sure. Do we need a new accessor on lineInfo which talks about UBA things instead of previous line? They may be backed by the same bool, but I wonder if we shouldn't have a UBA-specific accessor on lineInfo, since it's not clear from previousLineBrokeCleanly what it applies to... > Source/WebCore/rendering/style/RenderStyle.h:261 > - // 50 bits > + // 51 bits Did you validate this count? Sometimes these comments are out of date.

Comment on attachment 98088 [details] Patch Attachment 98088 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8926094 New failing tests: fast/text/international/unicode-bidi-plaintext.html

Created attachment 98093 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 98211 [details] Patch

Comment on attachment 98211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review > Source/WebCore/rendering/style/RenderStyle.h:261 > + // 53 bits It really is 53 bits now... The comment was off by 2!

Attachment 98211 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/RenderStyle.h:259: _unicodeBidi is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98211 [details] Patch Attachment 98211 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8925650 New failing tests: fast/text/international/unicode-bidi-plaintext.html

Created attachment 98215 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 98211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review r- for the cr-linux failure. Happy to look again after your replies. > Source/WebCore/html/HTMLElement.cpp:129 > +static inline int unicodeBidiAttributeForDirAuto(HTMLElement* element) So sad that these are ints, and not an enum. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:967 > + bool isNewUBAParagraph = lineInfo.previousLineBrokeCleanly(); I wonder if this shouldn't be an accessor on lineInfo. So that at some point later we might distinguish between nextLineIsNewUBAParagraph and previousLineBrokeCleanly (since I think they're used for separate things a times). > Source/WebCore/rendering/RenderBlockLineLayout.cpp:991 > + if (isNewUBAParagraph && style()->unicodeBidi() == Plaintext && !resolver.context()->parent()) { > + TextDirection direction = style()->direction(); > + determineParagraphDirection(direction, resolver.position()); > + resolver.setStatus(BidiStatus(direction, style()->unicodeBidi() == Override)); > + } Why is this needed here? I would think you would only need this in determinedStart... below.

Comment on attachment 98211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:967 >> + bool isNewUBAParagraph = lineInfo.previousLineBrokeCleanly(); > > I wonder if this shouldn't be an accessor on lineInfo. So that at some point later we might distinguish between nextLineIsNewUBAParagraph and previousLineBrokeCleanly (since I think they're used for separate things a times). I'd agree this should be refactored when another use case crops up. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:991 >> + } > > Why is this needed here? I would think you would only need this in determinedStart... below. On all other modes, hitting a hard line break resets the UBA to the state at the beginning of the current block, but when in Unicode: plaintext mode, we recalculate the direction of this new paragraph and set the status to an embedding level of 0/1 depending on the first character with strong directionality.

(In reply to comment #22) > (From update of attachment 98211 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98211&action=review > > r- for the cr-linux failure. Happy to look again after your replies. I'm actually really confused by this cr-linux failure... It's failing the new test I uploaded, but I don't have expected results for linux!

(In reply to comment #24) > I'm actually really confused by this cr-linux failure... It's failing the new test I uploaded, but I don't have expected results for linux! Can someone with a better understanding of the cr-linux ews bot's layout test functionality help me understand this? Eric, Adam, Dmitry, I'm looking at you ;)

You answered your own question, it failed because the new test you added doesn't have results for Chromium Linux. When the test runs, it falls back to the mac result and it doesn't match, so it fails. You can grab the result out of the zip file and add it to your patch and it should pass. Alternately you could skip the test on Chromium Linux.

(In reply to comment #26) > You answered your own question, it failed because the new test you added doesn't have results for Chromium Linux. When the test runs, it falls back to the mac result and it doesn't match, so it fails. > > You can grab the result out of the zip file and add it to your patch and it should pass. Alternately you could skip the test on Chromium Linux. Oof! That seems like like a false-negative to go red for missing platform test expectations. While I love having the expected results generated, it's quite annoying if I'm getting r-'s for not doing my building on Linux Chrome :p

Created attachment 98433 [details] Patch

(In reply to comment #28) > Created an attachment (id=98433) [details] > Patch Including text cr-linux results...

Attachment 98433 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/RenderStyle.h:259: _unicodeBidi is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 98433 [details] Patch Attachment 98433 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8935190 New failing tests: fast/text/international/unicode-bidi-plaintext.html

Created attachment 98439 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 98760 [details] Patch

Woo! Finally made cr-linux happy :)

Comment on attachment 98760 [details] Patch Seems OK. You will need to watch the bots, as you may break others than just linux due to font differences. I expect you'll break cr-win.

Comment on attachment 98760 [details] Patch Thanks for the review! I'll keep an eye on the bots, I'm sure there'll be different platform expectations :(

Comment on attachment 98760 [details] Patch Clearing flags on attachment: 98760 Committed r89864: <http://trac.webkit.org/changeset/89864>

All reviewed patches have been landed. Closing bug.