Bug 56814 – [Refactoring] Member variables of DocumentMarker should be encapsulated.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

Preparation for Bug 56812.

[Hajime Morrita]

Created attachment 93622 [details]
Patch

[Hajime Morrita]

Created attachment 93624 [details]
Patch

[Hajime Morrita]

This change is a preparation for DocumentMarker enhancement, 
which will allow DocumentMarkers to carry more detailed information about markers,
that includes not only existing descriptions and active-match flags, but also spelling suggestions.

[Tony Chang]

Comment on attachment 93624 [details]
Patch

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

> Source/WebCore/dom/DocumentMarker.h:128
> +inline DocumentMarker::DocumentMarker() 

Should we create a new .cpp file for these?

[Hajime Morrita]

Hi Tony, 
Thank you for reviewing this!
I'll land this shortly.

> > Source/WebCore/dom/DocumentMarker.h:128
> > +inline DocumentMarker::DocumentMarker() 
> 
> Should we create a new .cpp file for these?
Yes, that is my next step!

[Hajime Morrita]

Committed r86647: <http://trac.webkit.org/changeset/86647>

[Andrew Wilson]

Looks like this is causing crashes downstream:

ASSERTION FAILED: i < size()
third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h(537) : T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = WebCore::DocumentMarker, long unsigned int inlineCapacity = 0ul]

and

ASSERTION FAILED: newMarker.endOffset() >= newMarker.startOffset()

[Tony Chang]

(In reply to comment #7)
> Looks like this is causing crashes downstream:
> 
> ASSERTION FAILED: i < size()
> third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h(537) : T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = WebCore::DocumentMarker, long unsigned int inlineCapacity = 0ul]
> 
> and
> 
> ASSERTION FAILED: newMarker.endOffset() >= newMarker.startOffset()

Feel free to revert.

[Andrew Wilson]

Reverted r86647 for reason:

Broke tests downstream in Chromium

Committed r86704: <http://trac.webkit.org/changeset/86704>

[Tony Chang]

(In reply to comment #9)
> Reverted r86647 for reason:
> 
> Broke tests downstream in Chromium
> 
> Committed r86704: <http://trac.webkit.org/changeset/86704>

@atwilson: Which tests on which bots were broken?  That should help Morita in tracking down the failure.

[Andrew Wilson]

There were a ton of tests that failed. A small subset could be found here (this is just a single test runner's failures, but there were failures on nearly every browser_test runner):

http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%29%283%29/builds/90/steps/browser_tests/logs/stdio

I'm guessing that simply running browser_tests and ui_tests locally (or a trybot run) would be sufficient to show them all.

[Hajime Morrita]

(In reply to comment #11)
> There were a ton of tests that failed. A small subset could be found here (this is just a single test runner's failures, but there were failures on nearly every browser_test runner):
> 
> http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%29%283%29/builds/90/steps/browser_tests/logs/stdio
> 
> I'm guessing that simply running browser_tests and ui_tests locally (or a trybot run) would be sufficient to show them all.
Will take a look. I'm sorry for the break.

[Hajime Morrita]

Created attachment 93860 [details]
Patch

[Hajime Morrita]

The argument order of DocumentMarker constructor in addTextMatchMarker() was wrong.
Now it passes browser_tests with updated patch.

I hope we had layout tests for finding text...

[Darin Adler]

Comment on attachment 93860 [details]
Patch

If we change it like this, we should also change it from struct to class.

[Tony Chang]

Comment on attachment 93860 [details]
Patch

Feel free to convert to class in this change or a follow up change.

[Hajime Morrita]

(In reply to comment #15)
> (From update of attachment 93860 [details])
> If we change it like this, we should also change it from struct to class.
Sure. I'll covert this to a class, then land.

[Hajime Morrita]

Committed r86813: <http://trac.webkit.org/changeset/86813>

[Darin Adler]

This broke compiling with SUPPORT_AUTOCORRECTION_PANEL on because at least one call site was missed. I tried to fix it in <http://trac.webkit.org/changeset/86845>.

[Hajime Morrita]

(In reply to comment #19)
> This broke compiling with SUPPORT_AUTOCORRECTION_PANEL on because at least one call site was missed. I tried to fix it in <http://trac.webkit.org/changeset/86845>.
Darin, thank you for the fix. I missed it, again...