Closed Bug 1191356 Opened 9 years ago Closed 8 years ago

Clean up nsHTMLEditRules::RemoveListStructure, RemoveBlockContainer, MoveBlock, etc.

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(9 files, 9 obsolete files)

4.17 KB, patch
Details | Diff | Splinter Review
15.02 KB, patch
Details | Diff | Splinter Review
7.57 KB, patch
Details | Diff | Splinter Review
8.15 KB, patch
Details | Diff | Splinter Review
11.50 KB, patch
Details | Diff | Splinter Review
15.15 KB, patch
Details | Diff | Splinter Review
7.99 KB, patch
Details | Diff | Splinter Review
17.17 KB, patch
Details | Diff | Splinter Review
20.78 KB, patch
Details | Diff | Splinter Review
      No description provided.
Flags: in-testsuite-
The obvious ugly way to avoid the problem noted in the commit message is to make the new version have an ErrorResult, but that's really lame for something that can already return null.
Attachment #8645453 - Flags: review?(ehsan)
Attachment #8645450 - Flags: review?(ehsan)
Attachment #8645453 - Flags: review?(ehsan) → review+
Attachment #8645447 - Flags: review?(ehsan) → review+
Attachment #8645448 - Flags: review?(ehsan) → review+
Attachment #8645449 - Flags: review?(ehsan) → review+
Attachment #8645450 - Flags: review?(ehsan) → review+
Attachment #8645451 - Flags: review?(ehsan) → review+
Comment on attachment 8645452 [details] [diff] [review]
Part 6 -- Clean up nsHTMLEditRules::WillMakeBasicBlock

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

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +6712,5 @@
>  nsHTMLEditRules::SplitAsNeeded(nsIAtom& aTag,
> +                               OwningNonNull<nsINode>& aInOutParent,
> +                               int32_t& aInOutOffset)
> +{
> +  // XXX Is there a better way to do this?

I suppose we could write an adapter type from OwningNonNull to nsCOMPtr, but I wouldn't bother unless this comes up often...
Attachment #8645452 - Flags: review?(ehsan) → review+
Attachment #8645454 - Flags: review?(ehsan) → review+
Attachment #8645455 - Flags: review?(ehsan) → review+
Attached patch Part 1 updatedSplinter Review
Attachment #8645447 - Attachment is obsolete: true
Checkin instructions: This is a 46-patch set that needs to be checked in in the following order.  (Assume every patch depends on the patch before.)

1) Bug 1156062, patches 4-9.  (1-3 are already checked in.)

2) Bug 1190172, all patches (1-12).

3) Bug 1191354, all patches (1-13).

4) Bug 1191356, all patches (1-9).

5) Bug 1193762, patches 1-3.  (The other patches are not part of the series and I'll check them in myself later.)

This type of cleanup has caused regressions in the past, so to ease regression hunting, *every patch should be checked in in a separate push*, so mozregression will be able to test all of them.  (This means it should be on a weekend, apparently.)  

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e156c70327
(In reply to :Aryeh Gregor (working until May 8) from comment #20)
> 1) Bug 1156062, patches 4-9.  (1-3 are already checked in.)

This should be 4-12, not 4-9.
The first two bugs got checked in, but there was a misunderstanding, so they were checked in all at once and not one changeset per push.  Masayuki, are you able to push the rest this weekend?  I'm not available.  If not, I'll do it the Sunday after this one.
Flags: needinfo?(masayuki)
(In reply to :Aryeh Gregor (working until May 8) from comment #22)
> The first two bugs got checked in, but there was a misunderstanding, so they
> were checked in all at once and not one changeset per push.

How about to backout them and reland them separately?

> Masayuki, are
> you able to push the rest this weekend?  I'm not available.  If not, I'll do
> it the Sunday after this one.

I don't have the detail of plan of this weekend. I guess that I have some chance to land them, but I'm not sure I can do that for all patches.

Anyway, I strongly recommend that you should backout them for now.
Flags: needinfo?(masayuki) → needinfo?(ayg)
I wonder, do you really need to land your patches in weekend? I think that your patches are not so big and there are only a couple of developers hacking editor. So, you can land them even in a weekday.
The problem is that it overloads the builders, which slows down build times for everyone.  (Possibly it doesn't run more tests due to coalescing, but maybe it runs more tests too, I don't know.)  If it's on a weekend, 1) the builders are mostly idle anyway, and 2) hopefully if they're overloaded nobody's around to care.

I backed out the landings on inbound.
Flags: needinfo?(ayg)
By the way, the landings could be done with an at job, e.g.:

  $ at 2:00 Sunday
  at> cd /path/to/repo
  at> hg pull -u && while hg qpush; do hg qfin -a && hg push; done

I don't expect to be able to do it this week regardless, but maybe someone else can.  If not, I should be able to do it next week.
How about to land 3-4 patches per day? It's not so many landings per developer.
(In reply to :Aryeh Gregor (working until May 8) from comment #20)
> Checkin instructions: This is a 46-patch set that needs to be checked in in
> the following order.  (Assume every patch depends on the patch before.)
> 
> 1) Bug 1156062, patches 4-9.  (1-3 are already checked in.)
> 
> 2) Bug 1190172, all patches (1-12).
> 
> 3) Bug 1191354, all patches (1-13).
> 
> 4) Bug 1191356, all patches (1-9).
> 
> 5) Bug 1193762, patches 1-3.  (The other patches are not part of the series
> and I'll check them in myself later.)
> 
> This type of cleanup has caused regressions in the past, so to ease
> regression hunting, *every patch should be checked in in a separate push*,
> so mozregression will be able to test all of them.  (This means it should be
> on a weekend, apparently.)  
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0e156c70327

> patching file editor/libeditor/nsHTMLEditRules.cpp
> Hunk #3 FAILED at 2426
> 1 out of 4 hunks FAILED -- saving rejects to file editor/libeditor/nsHTMLEditRules.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh bug1156062-8.diff

The patch of bug 1156062 part 8 is rejected with current mozilla-inbound. Please make sure all patches can be applied to the latest mozilla-inbound by next morning.
Flags: needinfo?(ayg)
FYI: Perhaps, I can work on landing the patches for a couple of hours about 24 hours later from now. Although, I'm still not sure if I can work on Sunday.
Flags: needinfo?(ayg)
A rebase failed and I somehow didn't notice, leaving a merge marker in the file.  I resolved the conflict, compile-tested the remaining seven patches locally, and repushed to inbound.
Flags: needinfo?(ayg)
Seems to have stuck.  One commit still failed to build (somehow I pushed the broken version instead of the corrected one), but two commits later it starts to build, so that range of three commits will not be bisectable.
You need to log in before you can comment on or make changes to this bug.