Place inner frame scrollbars on the left side, if indicated by the frame-level text direction.

Place inner frame scrollbars on the left side, if indicated by the frame-level
text direction.

CL overrides ScrollView::shouldPlaceVerticalScrollbarOnLeft() from FrameView in
order to determine if a Frame level scrollbar should be placed on the left.
Logic is simple: it obeys what ever is set to the body node, as it determines
the page text direction.

For now, it only covers inner frames, as main frame level scrollbar placement
logic should be dictated by the application/system (see crbug.com/249860).

Patch does not introduce new tests, but marks the existing affected tests as
NeedsManualRebaseline in order to get
results for all platforms from the bots, and perform rebaselines accordingly.
Once this is done, idea is to re-mark them as flaky.

Bug=250514
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169180

[tonikitoo_, 2014-02-12 01:04:20]

The CQ bit was checked by a1.gomes@sisa.samsung.com

[tonikitoo_, 2014-02-12 01:04:21]

The CQ bit was unchecked by a1.gomes@sisa.samsung.com

[Julien - ping for review, 2014-02-12 23:25:46]

The description should follow git commit guideline:
- short 1-line description
- longer explanations (though less than 80 (72 if you want to account for
rollouts - see
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY))

https://codereview.chromium.org/150733002/diff/200001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/150733002/diff/200001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:153: crbug.com/250514
virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-scrolled.html [
NeedsManualRebaseline ]
FYI you can also add a new entry for the rebaseline as long as it doesn't
conflict with an existing entry.

That would avoid the 2 patches dance.

https://codereview.chromium.org/150733002/diff/200001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/150733002/diff/200001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1334: // Mainframe scrollbar placement should
respect the embedding application RTL UI policy.
That looks like a FIXME related to crbug.com/249860

[tonikitoo_, 2014-03-11 17:03:59]

On 2014/02/12 23:25:46, Julien Chaffraix - PST wrote:
> The description should follow git commit guideline:
> - short 1-line description
> - longer explanations (though less than 80 (72 if you want to account for
> rollouts - see
>
https://groups.google.com/a/chromium.org/forum/#%21topic/blink-dev/rqGhY7PfkYY))
> 
>
https://codereview.chromium.org/150733002/diff/200001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/150733002/diff/200001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:153: crbug.com/250514
> virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow-scrolled.html [
> NeedsManualRebaseline ]
> FYI you can also add a new entry for the rebaseline as long as it doesn't
> conflict with an existing entry.
> 
> That would avoid the 2 patches dance.

I particularly feel like it goes smoother this way, but let me know if you have
a strong opinion about it, please.

Other comments are addressed. Please review.

[tonikitoo_, 2014-03-12 21:28:05]

Context about why I am handling the tests this way in the patch:

Feb 11 14:07:43 <ojan>	tonikitoo: what's up?
Feb 11 14:08:03 <tonikitoo>	ojan, hi sir, I might need some quick guidance on
how to handle rebasilines on https://codereview.chromium.org/150733002/
Feb 11 14:08:20 <tonikitoo>	ojan,  basically it requires rebasiline (text+image)
of some tests that are already marked as NeedsManualRebasiline.
Feb 11 14:08:43 <tonikitoo>	ojan, I thougth I could: 1) mark them as
NeedsRebasiline, 2) wait for the bot cycle, 3) then mark them as
NeedsManualRebasiline again (with the new results)
Feb 11 14:08:45 <tonikitoo>	Does it sound practical?
Feb 11 14:09:37 <ojan>	tonikitoo: it might be easier to just do the manual
rebaseline first and get rid of the NeedsManualRebaseline lines...
Feb 11 14:09:49 <ojan>	tonikitoo: that said...i wonder why they needed to be
manual in the first place
Feb 11 14:10:06 <tonikitoo>	ojan, that was my first thought. there is some
history behind it though
Feb 11 14:10:07 <ojan>	tonikitoo: probably because they conflicted with other
lines that need to be commented back in once the rebaselines are completed
Feb 11 14:10:27 <ojan>	tonikitoo: can you point me to the patch that added the
NeedsManualRebaseline lines
Feb 11 14:10:44 <ojan>	tonikitoo: there's no good way to handle this situation.
:(
Feb 11 14:11:07 <ojan>	this is one of the reasons i tried so hard a few months
back to get TestExpectations empty
Feb 11 14:11:16 <ojan>	every line we have in there is a maintenance burden
Feb 11 14:11:59 <tonikitoo>	ojan, here is what is in the TestExpectations:
Feb 11 14:12:00 <tonikitoo>	 129 # Pixel tests for RTL iframe scrollbar is
erroneous. Cannot observe on actual browser.
Feb 11 14:12:00 <tonikitoo>	 130 crbug.com/338794 [ Mac ]
compositing/rtl/rtl-iframe-absolute-overflow.html [ NeedsManualRebaseline ]
Feb 11 14:12:00 <tonikitoo>	 131 crbug.com/338794 [ Mac ]
compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html [
NeedsManualRebaseline ]
Feb 11 14:12:01 <tonikitoo>	...
Feb 11 14:13:08 <tonikitoo>	ojan, so it seems the results are flaky, 'till
crbug.com/338794 is fixed.
Feb 11 14:15:47 <ojan>	tonikitoo: ok...i think those should just not be marked
NeedsManualRebaseline...they should be marked [ Pass Failure ] like they are on
every other platform
Feb 11 14:16:16 <tonikitoo>	agreed. I imagine NeedsManualRebaseline was "abused"
here, and used as a way to say: I need proper results, when bug xxx is fixed.
Feb 11 14:16:41 <ojan>	tonikitoo: which...unfortunately for you...means that
your patch will need to comment out the flaky lines, add NeedsManualRebaseline
lines...wait for the bots to cycle...do the rebaselines and uncomment out the
flaky lines again
Feb 11 14:16:51 <ojan>	it's a big pain :(
Feb 11 14:17:26 <ojan>	it's jsut two patches though...your original patch + the
rebaseline/uncomment patch
Feb 11 14:18:02 <tonikitoo>	I see, ojan. is that preferrable than that I first
suggested?
Feb 11 14:18:03 <tonikitoo>	can I replace NeedsManualRebaseline by
NeedsRebaseline, get the results updated, and then mark them as Pass Fail?
Feb 11 14:18:56 <ojan>	tonikitoo: the problem with that is that the bot will
automatically commit and remove the NeedsRebaseline lines...so there will be a
window where the tests are flaky but not marked as such in TestExpectations
Feb 11 14:19:43 <ojan>	tonikitoo: incidentally...doing the rebaseline manually
isn't that hard...s/NeedsManualRebaseline/Rebaseline/ in TestExpectations and
then run "webkit-patch rebaseline-expectations"
Feb 11 14:19:53 <ojan>	tonikitoo: that's what the NeedsRebaseline bot does under
the hood anyways
Feb 11 14:20:01 <tonikitoo>	I see.
Feb 11 14:20:16 <ojan>	tonikitoo: the only issue is that you have to make sure
that all the bots have cycled before you do this
Feb 11 14:20:31 <tonikitoo>	what worried me if that I did not know if it handled
multiple platforms well
Feb 11 14:21:27 <tonikitoo>	for example, these tests have cross-platform
expectations, as well as platform specific ones, platform-flaky ones, and
platform-needsManualRebaseline-ones
Feb 11 14:22:47 <tonikitoo>	ojan, so having the window of flakyness might be as
bad as doing the manual rebasilines myself. I will have to keep looking at the
bots in both cases, as far as I can understand
Feb 11 14:23:16 <ojan>	tonikitoo: but it might might make the gardener's life
more difficult as the tree will be going red
Feb 11 14:24:30 <tonikitoo>	I see. let me read your suggestion again, and
understand why tree would not go red

[Julien - ping for review, 2014-03-12 23:19:09]

lgtm (assuming the new baselines are progressing)

https://codereview.chromium.org/150733002/diff/280001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/150733002/diff/280001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1397: if (!document->body() ||
!document->body()->renderer())
I would put a comment that 'direction' is inherited so it's better to use <body>
that actually <html> to determine it.

https://codereview.chromium.org/150733002/diff/280001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1401: return
document->body()->renderer()->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft();
shouldPlaceBlockDirectionScrollbarOnLogicalLeft is really misnamed based on the
fact that it only handles the RTL horizontal writing mode case (logical left is
physical right and the block direction is horizontal so those are vertical
scrollbars).

It would be nice to rename it as part of this patch to something better but I
couldn't think of a better name.

[tonikitoo_, 2014-03-13 23:07:32]

The CQ bit was checked by a1.gomes@sisa.samsung.com

[commit-bot: I haz the power, 2014-03-13 23:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/150733002/30...

[commit-bot: I haz the power, 2014-03-13 23:07:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:07:48]

Failed to apply patch for Source/core/frame/FrameView.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/FrameView.h
  Hunk #1 FAILED at 297.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/frame/FrameView.h.rej

Patch:       Source/core/frame/FrameView.h
Index: Source/core/frame/FrameView.h
diff --git a/Source/core/frame/FrameView.h b/Source/core/frame/FrameView.h
index
da37a9d36a71fa4826fc502c44e25ce03d88e47b..ea2dcfa2505e0a9f36076c7cf2b97e037136052f
100644
--- a/Source/core/frame/FrameView.h
+++ b/Source/core/frame/FrameView.h
@@ -297,6 +297,8 @@ public:
     // DEPRECATED: Use viewportConstrainedVisibleContentRect() instead.
     IntSize scrollOffsetForFixedPosition() const;
 
+    virtual bool shouldPlaceVerticalScrollbarOnLeft() const OVERRIDE;
+
     PartialLayoutState& partialLayout() { return m_partialLayout; }
 
     // Override scrollbar notifications to update the AXObject cache.

[tonikitoo_, 2014-03-13 23:26:17]

The CQ bit was checked by a1.gomes@sisa.samsung.com

[commit-bot: I haz the power, 2014-03-13 23:26:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a1.gomes@sisa.samsung.com/150733002/31...

[commit-bot: I haz the power, 2014-03-14 00:00:31]

Change committed as 169180