1048752 - Clean up nsCaret

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I was trying to fix bug 1001077 but was going nowhere because nsCaret is a nightmare. It's a mass of undocumented state with undocumented (and very confusing) invariants. There's also a lot of cruft that became unnecessary with DLBI and other changes. I have a set of patches to massively clean it up. The basic plan is to compute most of what we need (caret node, caret frame, caret rect) at paint time and keep only the minimal amount of state around at other times.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cb4bec1a0e4f

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=a84911e7c524 is looking good except for M4 (debug) which has a failure, but no indication in the log *what* failed.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

tnikkel, I choose you!

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> https://tbpl.mozilla.org/?tree=Try&rev=a84911e7c524 is looking good except
> for M4 (debug) which has a failure, but no indication in the log *what*
> failed.

Turns out it's because of an assertion firing on trunk that my patches fixed, so I just need to update the test's assertion annotation.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Remove GetCaretReadOnly (unused)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Remove InvalidateOutsideCaret and MaybeInvalidateCaretPosition

I'm quite sure DLBI means we don't need these anymore.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Make GetCaretFrameForNodeOffset static

GetCaretFrameForNodeOffset only uses the nsFrameSelection and mBidiUI
from its nsCaret. For mBidiUI we can just get the pref directly whenever
we need it. By modifying GetCaretFrameForNodeOffset to take nsFrameSelection
as a parameter, we can make it static to make it clear it isn't really
related to the state of the nsCaret.

This may also fix a bug in Selection::GetPrimaryFrameForFocusNode where
things would go unexpectedly wrong if mCaret is temporarily observing
a different selection to the Selection.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Make EViewCoordinates (unused)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Reorder API declarations

To make it more clear what's going on, make CaretBlinkCallback protected
and reorder the public methods to put the state-changing "API" methods first.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Remove 'virtual' declarations

These used to be needed for linking with non-libxul builds, but are no
longer needed. The obsolete comment is fixed in a later patch.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: Remove mCaretWidthCSSPx

mCaretWidthCSSPx doesn't need to be stored. We can just recompute it
whenever we need it.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: Remove mCaretAspectRatio

mCaretAspectRatio doesn't need to be stored. We can recompute it
whenever we need it.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: Remove nsCaret.h from nsDisplayList.h

I got tired of slow build turnarounds every time I modified nsCaret.h.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 10: Remove FrameLayerBuilder.h from nsDisplayList.h

I was inspired by the previous patch to remove FrameLayerBuilder.h from
nsDisplayList.h too.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 11: Remove nsLayoutUtils.h from nsDisplayList.h

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 12: Replace mBlinkRate with mIsBlinking

We don't need to store the blink rate. Instead we can just fetch it whenever
we need it. However we do need a flag to handle the case where
nsCaret::DrawAtPosition disables blinking.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 13: Remove GetGeometry's aBidiIndicatorSize

Callers always pass null.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 14: Split GetGeometry into a static and a non-static version

GetGeometry is used in two different ways. Sometimes it's used to get
information about a particular caret. Sometimes it's used to get
information about a particular selection that's not associated with
a caret. Splitting GetGeometry into a non-static version for the former
and a static version for the latter makes this more clear. Also it saves
code since for the latter version we don't have to get an nsCaret first.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15: Fix some comments

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 16: Move nsFrameSelection::HINT to CaretAssociationHint.h

This patch started an attempt to remove nsFrameSelection.h from nsCaret.h
and metastasized into a rather large refactoring patch that removed it
from some other header files as well, and changed nsFrameSelection::HINT
into a global-scope enum with better names. I also converted bools
into CaretAssociationHint in a few places where that was appropriate,
but there are still some more places (GetChildFrameContainingOffset)
where bools need to be converted. I figured this patch was big enough already.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 17: Rename Get/SetCaretDOMSelection to Get/SetSelection

The forward declaration of Selection in nsCaret.h will be used in later patches.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 18: Add nsCaret::SchedulePaint

We'll use this later.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 19: Create nsCaret::GetPaintGeometry to call during painting

This is the start of the changes to caret-drawing proper.

The idea is to combine GetCaretFrame and GetCaretRect into a method
GetPaintGeometry which looks like GetGeometry but returns values
needed for painting (i.e. including bidi decorations, and returning
a null frame if we're not supposed to paint due to specific caret
state, e.g. in the "off" phase of the blink cycle).

Mostly a straightforward refactoring but there are a few interesting changes:
-- nsDisplayCaret stores its bounds instead of getting them from nsCaret on
demand. Eventually those bounds will not be stored in nsCaret at all.
-- nsDisplayCaret::GetBounds returns true for aSnap. nsCaret draws snapped
rects, so why not.
-- I removed "if (caretRect.Intersects(aDirtyRect))" in EnterPresShell.
As far as I can tell, this check is incorrect because it doesn't take
transforms into account. Since there's at most one drawn caret per window,
hence we do this at most once per paint, I don't think there's any real
performance advantage to having this check.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 20: Inline GetCaretFrame into its callers

This duplicates some code, but later patches will modify the callers and then
eventually we'll re-share common code.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 21: Instead of storing mCaretRect/mHookRect, recompute them whenever they're needed

Also, moves the "If the offset falls outside of the frame" check from
PaintCaret to GetPaintGeometry so we do less work in that case.

UpdateCaretRects is no longer needed.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 22: Factor out SelectionLanguageChange call to run early in nsCaret::GetPaintGeometry

This code is somewhat tricky. nsCaret::ComputeCaretRects was checking to see
if we have a bidi keyboard, and if so, what direction it's set to.
If the direction changed from the last direction seen for *this caret*,
we fired a SelectionLanguageChange notification on the caret's current
Selection. This looked bogus because the caret can be switched between
selections so it would seem some selections won't get a notification when
they should, but that's how it was. Also, when the SelectionLanguageChange
notification fired we then didn't draw the caret in that iteration, which
seems even more bogus.

This patch fixes all that by moving the logic to fire SelectionLanguageChange
out to GetPaintGeometry and firing the notification every single time without
trying to detect whether the state has changed or not. I carefully examined
the implementation of SelectionLanguageChange and I'm pretty sure it's
idempotent so this should be correct. That doesn't look like an
expensive function, and runs at most once per window paint, so I'm not
worried about perf. Because we now fire SelectionLanguageChange before
reading selection or frame state, it should be fine to carry on after
calling SelectionLanguageChange and drawing the caret based on whatever
changes SelectionLanguageChange has performed.

This also lets us remove mKeyboardRTL, which as noted above seems inherently
bogus.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 23: Remove aIgnoreDrawnState from MustDrawCaret

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 24: Rename and deCOM Set/GetCaretVisible

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 25: Inline MustDrawCaret into IsVisible

This also fixes what appears to be a bug. MustDrawCaret returned true
when mShowDuringSelection is set even if the caret would otherwise be
hidden due a popup showing. That doesn't make sense.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 26: Move all "should draw caret" responsibility to GetPaintGeometry

Instead of checking the mysterious mDrawn state (which is evil and will be
removed), let nsCaret::GetPaintGeometry take sole responsibilty for deciding
whether to draw. It takes the nsStyleUserInterface checks. It also needs to
check blink state, which is made possible by separating blink state into
the mIsBlinkOn flag.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 27: Remove obsolete caret state logic and just SchedulePaint as needed

This is the core of the whole patch set.

Now GetPaintGeometry/PaintCaret figure out on their own almost all the state
they need every time we paint. So when caret flags change, all we need
to do is SchedulePaint. We don't need to fiddle with mDrawn and most of the
logic in DrawCaret is obsolete. (In fact, it was duplicated by GetGeometry
and friends, and we're removing that duplication.) EraseCaret, CheckCaretState
and UpdateCaretPosition are also obsolete.

We need to have GetPaintGeometry/PaintCaret choose the correct content node
and offset, either getting them from the Selection or using specific data set by
DrawAtPosition. This logic, plus a bit of other code shared between them, is
put into the helper GetFrameAndOffset.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 28: Remove unused fields and rename mLast* to mOverride*

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 29: mVisible is never true in nsCaret::Init

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 30: Simplify caret blinking logic

A few things got mashed together here:
-- Inline KillTimer/PrimeTimer into their callers.
-- Instead of having to call StopBlinking and StartBlinking together,
change StartBlinking to ResetBlinking and have it set up
the correct blink state and reset the blink cycle.
-- nsCaret::NotifySelectionChange needs a SchedulePaint
-- nsCaret::DrawAtPosition needs a ResetBlinking

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 31: nsCaret.h must be the first #include in nsCaret.cpp

Also cleans up a couple of other ordering issues.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 32: Rename DrawAtPosition to SetCaretPosition and don't make it turn off blinking

There's no need for this method to turn off blinking anymore. Its only
caller already calls SetCaretReadOnly to achieve the same effect. That
means we don't actually need the mIsBlinking flag after all.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 33: A zero blink rate should mean the caret is always on

Previous changes broke this.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 34: Handle null selection returned by GetSelection()

Previous changes broke this. Tests caught it.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 35: Remove GetFrameSelection (unused)

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 36: Remove ancient horizontal rules

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 37: GetGeometryForFrame should return a rect instead of an nsresult

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 38: Adjust test annotation for fixed assertion

This test used to assert because we tried to get frame geometry during a caret
update while the frame was dirty. That no longer happens since now we only try
to get frame geometry during a paint. During a paint, frames can still be dirty
so this assertion could still be triggered, and we should probably fix that
eventually, but it would only happen during an interrupted layout which is
quite rare.

Comment 42

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8468815 [details] [diff] [review]
Part 12: Replace mBlinkRate with mIsBlinking

On Windows (and Linux) getting the blink rate from the LookAndFeel is a system call. Are we calling this enough that we want to avoid the system call?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Timothy Nikkel (:tn) from comment #42)
> On Windows (and Linux) getting the blink rate from the LookAndFeel is a
> system call. Are we calling this enough that we want to avoid the system
> call?

StartBlinking/ResetBlinking should not be called very often. It could be called in response to JS changes to the selection, so someone could write a benchmark around that I suppose, but it wouldn't be very realistic to change the selection hundreds of times a second.

GetCaretBlinkTime is a Windows API call. What makes you think it's a system call, though?

Comment 44

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> StartBlinking/ResetBlinking should not be called very often. It could be
> called in response to JS changes to the selection, so someone could write a
> benchmark around that I suppose, but it wouldn't be very realistic to change
> the selection hundreds of times a second.

Okay.

> GetCaretBlinkTime is a Windows API call. What makes you think it's a system
> call, though?

I meant the former, sorry.

Comment 45

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Hi Robert, 

I'm working on bug 1016184 to enable touch caret preference by default on B2G. I've already fixes all the gecko test failures. However, due to intermittent Gaia ui test fail, I could not land the final patch to turn on the preference. Currently, all the tests cases are run without touch caret on.

I'm happy to see the mess in nsCaret be clean up. Since touch caret depends on caret in many ways, I would like to know if the touch caret works well with your patches. Would you please turn on touch caret on b2g while testing your patches on try temporarily. Perhaps we could find some potential issues.

Comment 46

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8468819 [details] [diff] [review]
Part 16: Move nsFrameSelection::HINT to CaretAssociationHint.h

>-  virtual nsresult GetChildFrameContainingOffset(int32_t    inContentOffset,
>-                                                 bool       inHint,//false stick left
>-                                                 int32_t*   outFrameContentOffset,
>-                                                 nsIFrame** outChildFrame) = 0;
>+  virtual nsresult GetChildFrameContainingOffset(int32_t            inContentOffset,
>+                                                 bool inHint,
>+                                                 int32_t*           outFrameContentOffset,
>+                                                 nsIFrame**         outChildFrame) = 0;

Since you're here, fix the inHint indent.

Comment 47

[Simon Montagu :smontagu]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> Created attachment 8468825 [details] [diff] [review]
> Part 22: Factor out SelectionLanguageChange call to run early in
> nsCaret::GetPaintGeometry

> Also, when the SelectionLanguageChange
> notification fired we then didn't draw the caret in that iteration, which
> seems even more bogus.

It was always ugly, q.v. bug 265061, but AFAIR I didn't find any other way to prevent caret turds when changing language. If moving this earlier solves that problem, great!

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #45)
> I'm happy to see the mess in nsCaret be clean up. Since touch caret depends
> on caret in many ways, I would like to know if the touch caret works well
> with your patches. Would you please turn on touch caret on b2g while testing
> your patches on try temporarily. Perhaps we could find some potential issues.

There is one failure on Mulet M5:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45502132&tree=Try&full=1
20:57:11     INFO -  618 INFO TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_reftests_with_caret.html | Reftest http://mochi.test:8888/tests/layout/base/tests/bug512295-1.html FAILED
20:57:12     INFO -  619 INFO TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_reftests_with_caret.html | Reftest http://mochi.test:8888/tests/layout/base/tests/bug512295-2.html FAILED
Basically, in those tests, the caret is in the right position, but the touchcaret is not drawn at all.

Apart from that, the full try push looks pretty good:
https://tbpl.mozilla.org/?tree=Try&rev=2694148f474b

There's a test where I fixed an assertion on Mac and Windows, so I just need to adjust the test annotation.

The only other issue is a failure on Mac:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45500607&tree=Try&full=1#error1
20:53:46     INFO -  2389 INFO TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_reftests_with_caret.html | Reftest http://mochi.test:8888/tests/layout/base/tests/bug646382-1.html FAILED
20:53:46     INFO -  2390 INFO TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_reftests_with_caret.html | Reftest http://mochi.test:8888/tests/layout/base/tests/bug646382-2.html FAILED

Not really sure what happened there. Guess I'll have to get a build going on Mac.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Simon Montagu :smontagu from comment #47)
> It was always ugly, q.v. bug 265061,

That bug should be quite easy to fix on top of these patches.

Comment 50

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8468827 [details] [diff] [review]
Part 24: Rename and deCOM Set/GetCaretVisible

Looks like a hunk from part 14 got in here.

Comment 51

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8468830 [details] [diff] [review]
Part 27: Remove obsolete caret state logic and just SchedulePaint as needed

Why don't we need to call SchedulePaint in StopBlinking?

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Timothy Nikkel (:tn) from comment #51)
> Comment on attachment 8468830 [details] [diff] [review]
> Part 27: Remove obsolete caret state logic and just SchedulePaint as needed
> 
> Why don't we need to call SchedulePaint in StopBlinking?

By the end of this patch set, StopBlinking is only called when shutting down the presshell or by ResetBlinking. All callers of ResetBlinking call SchedulePaint as well.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 39: Call SelectionLanguageChange even when bidi keyboard is not available, for consistency, and fix broken tests

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8471901 [details] [diff] [review]
Part 39: Call SelectionLanguageChange even when bidi keyboard is not available, for consistency, and fix broken tests

Review of attachment 8471901 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/tests/bug646382-1-ref.html
@@ +9,5 @@
>        function start() {
>          textarea.focus();
>        }
>        function done() {
> +        textarea.selectionStart = textarea.selectionEnd = 1;

As far as I can tell, bug646382-1.html and bug646382-2.html actually move the caret to the end of the text, and the tests only pass because of some caret bug that I've fixed by calling SelectionLanguageChanged in more cases --- but only when a bidi keyboard is present, which apparently is only on Mac test machines. So I was getting a reftest failure in these tests on Mac.

Making these test changes fixed the failure on Mac, and they seem correct to me, but I'm not sure hence asking for review.

These test changes caused failure on other platforms, I think because SelectionLanguageChanged wasn't getting called due to no bidi keyboard. So I made us call SelectionLanguageChange whether the bidi keyboard is present or not.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8471901 [details] [diff] [review]
Part 39: Call SelectionLanguageChange even when bidi keyboard is not available, for consistency, and fix broken tests

Review of attachment 8471901 [details] [diff] [review]:
-----------------------------------------------------------------

Jonathan, maybe you can review this instead of Simon?

Comment 56

[Jonathan Kew [:jfkthame]]

Comment on attachment 8471901 [details] [diff] [review]
Part 39: Call SelectionLanguageChange even when bidi keyboard is not available, for consistency, and fix broken tests

Review of attachment 8471901 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCaret.cpp
@@ +842,2 @@
>    if (bidiKeyboard && NS_SUCCEEDED(bidiKeyboard->IsLangRTL(&isCaretRTL)) &&
>        IsBidiUI()) {

Might it be tidier to check !IsBidiUI() before this bidiKeyboard stuff, and bail out earlier (a bit like you do in CheckSelectionLanguageChange() above)?

::: layout/base/tests/bug646382-1-ref.html
@@ +9,5 @@
>        function start() {
>          textarea.focus();
>        }
>        function done() {
> +        textarea.selectionStart = textarea.selectionEnd = 1;

I agree, those testcases leave the caret at the end of the text, and the existing references are wrong. They seem to be passing because of an odd bug in our current behavior, whereby after the second VK_DOWN event in the tests, the caret visually jumps back to the start of the text -- but typing another character at that point confirms that in fact the selection is still at the end. So this seems to be an existing caret-display bug that you're fixing here.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/030449cb7fed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ed6f49d404
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e3daef2400
https://hg.mozilla.org/integration/mozilla-inbound/rev/52109d5178c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e94c0c00ffed
https://hg.mozilla.org/integration/mozilla-inbound/rev/49bf30b05103
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f6fe7ec8fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f8c936d763
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4a52e44188
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca58eced657
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b885c741dc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5761be456996
https://hg.mozilla.org/integration/mozilla-inbound/rev/744443e8760a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca077bb6fbc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b56aabc823
https://hg.mozilla.org/integration/mozilla-inbound/rev/a48a0eb4946f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e96157e3af4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8e4df32ffc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2826371a64df
https://hg.mozilla.org/integration/mozilla-inbound/rev/c550ff16a9a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/359d7c9c767a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea929ba6a8fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5693a24f45a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a7988dcbe9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed02962f30eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/760ba706ad90
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c6ddbb2519
https://hg.mozilla.org/integration/mozilla-inbound/rev/399e64e0c4ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b040c9158f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9c04a9200b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b948d2bfea70
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1857632271d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ab5b511d88
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2a05a50eec
https://hg.mozilla.org/integration/mozilla-inbound/rev/3181703ca8a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e732e0b3a059
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ebca84edc23
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f357d9f3cdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f016da18b899

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The latest try push was here: https://tbpl.mozilla.org/?tree=Try&rev=4b1bf6d3ca70

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Something in this push appears to have broken Windows non-unified builds. Since it's the weekend and things are slow, I'm willing to let you fix in place, but this does block the next inbound -> m-c merge, so please look ASAP.

https://tbpl.mozilla.org/php/getParsedLog.php?id=46101590&tree=Mozilla-Inbound

If you need to reproduce locally or on Try, --disable-unified-compilation is the needed mozconfig option.

Comment 60

[Jonathan Kew [:jfkthame]]

Attachment: followup - fix for Windows non-unified build. no_ bustage fix

As I think :roc ought to be asleep at the moment, I'm intending to push this patch, which just adds the nsRange.h header to sdnTextAccessible.cpp; I assume it was previously being indirectly included via some other header. AFAICS this should fix the Windows non-unified bustage; feel free to back it out if I'm wrong.

Comment 61

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2793df6207c6

Comment 62

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jonathan Kew (:jfkthame) from comment #61)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2793df6207c6

Appears to have fixed the bustage from comment 59, but now it's hitting this instead :(

https://tbpl.mozilla.org/php/getParsedLog.php?id=46103903&tree=Mozilla-Inbound

Comment 63

[Jonathan Kew [:jfkthame]]

Pushed a further bustage fix (presumed, based on the log):
https://hg.mozilla.org/integration/mozilla-inbound/rev/4173a2b47c5a

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Victory! Thanks for the follow-up :)

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/030449cb7fed
https://hg.mozilla.org/mozilla-central/rev/f2ed6f49d404
https://hg.mozilla.org/mozilla-central/rev/a8e3daef2400
https://hg.mozilla.org/mozilla-central/rev/52109d5178c9
https://hg.mozilla.org/mozilla-central/rev/e94c0c00ffed
https://hg.mozilla.org/mozilla-central/rev/49bf30b05103
https://hg.mozilla.org/mozilla-central/rev/d7f6fe7ec8fe
https://hg.mozilla.org/mozilla-central/rev/86f8c936d763
https://hg.mozilla.org/mozilla-central/rev/3f4a52e44188
https://hg.mozilla.org/mozilla-central/rev/1ca58eced657
https://hg.mozilla.org/mozilla-central/rev/7b885c741dc9
https://hg.mozilla.org/mozilla-central/rev/5761be456996
https://hg.mozilla.org/mozilla-central/rev/744443e8760a
https://hg.mozilla.org/mozilla-central/rev/ca077bb6fbc3
https://hg.mozilla.org/mozilla-central/rev/77b56aabc823
https://hg.mozilla.org/mozilla-central/rev/a48a0eb4946f
https://hg.mozilla.org/mozilla-central/rev/e96157e3af4d
https://hg.mozilla.org/mozilla-central/rev/8d8e4df32ffc
https://hg.mozilla.org/mozilla-central/rev/2826371a64df
https://hg.mozilla.org/mozilla-central/rev/c550ff16a9a2
https://hg.mozilla.org/mozilla-central/rev/359d7c9c767a
https://hg.mozilla.org/mozilla-central/rev/ea929ba6a8fc
https://hg.mozilla.org/mozilla-central/rev/d5693a24f45a
https://hg.mozilla.org/mozilla-central/rev/e1a7988dcbe9
https://hg.mozilla.org/mozilla-central/rev/ed02962f30eb
https://hg.mozilla.org/mozilla-central/rev/760ba706ad90
https://hg.mozilla.org/mozilla-central/rev/b0c6ddbb2519
https://hg.mozilla.org/mozilla-central/rev/399e64e0c4ba
https://hg.mozilla.org/mozilla-central/rev/82b040c9158f
https://hg.mozilla.org/mozilla-central/rev/bd9c04a9200b
https://hg.mozilla.org/mozilla-central/rev/b948d2bfea70
https://hg.mozilla.org/mozilla-central/rev/e1857632271d
https://hg.mozilla.org/mozilla-central/rev/b2ab5b511d88
https://hg.mozilla.org/mozilla-central/rev/6b2a05a50eec
https://hg.mozilla.org/mozilla-central/rev/3181703ca8a9
https://hg.mozilla.org/mozilla-central/rev/e732e0b3a059
https://hg.mozilla.org/mozilla-central/rev/9ebca84edc23
https://hg.mozilla.org/mozilla-central/rev/0f357d9f3cdd
https://hg.mozilla.org/mozilla-central/rev/f016da18b899
https://hg.mozilla.org/mozilla-central/rev/2793df6207c6
https://hg.mozilla.org/mozilla-central/rev/4173a2b47c5a