Bug 56814 - [Refactoring] Member variables of DocumentMarker should be encapsulated.
Summary: [Refactoring] Member variables of DocumentMarker should be encapsulated.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 56812 58111
  Show dependency treegraph
 
Reported: 2011-03-22 02:03 PDT by Hajime Morrita
Modified: 2011-05-19 20:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (38.52 KB, patch)
2011-05-16 01:42 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (38.53 KB, patch)
2011-05-16 01:45 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (38.52 KB, patch)
2011-05-17 20:22 PDT, Hajime Morrita
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-03-22 02:03:55 PDT
Preparation for Bug 56812.
Comment 1 Hajime Morrita 2011-05-16 01:42:59 PDT
Created attachment 93622 [details]
Patch
Comment 2 Hajime Morrita 2011-05-16 01:45:19 PDT
Created attachment 93624 [details]
Patch
Comment 3 Hajime Morrita 2011-05-16 01:46:49 PDT
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.
Comment 4 Tony Chang 2011-05-16 10:00:01 PDT
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?
Comment 5 Hajime Morrita 2011-05-16 17:47:51 PDT
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!
Comment 6 Hajime Morrita 2011-05-16 18:11:32 PDT
Committed r86647: <http://trac.webkit.org/changeset/86647>
Comment 7 Andrew Wilson 2011-05-17 11:35:47 PDT
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()
Comment 8 Tony Chang 2011-05-17 11:37:56 PDT
(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.
Comment 9 Andrew Wilson 2011-05-17 13:39:28 PDT
Reverted r86647 for reason:

Broke tests downstream in Chromium

Committed r86704: <http://trac.webkit.org/changeset/86704>
Comment 10 Tony Chang 2011-05-17 13:47:37 PDT
(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.
Comment 11 Andrew Wilson 2011-05-17 13:54:34 PDT
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.
Comment 12 Hajime Morrita 2011-05-17 17:40:28 PDT
(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.
Comment 13 Hajime Morrita 2011-05-17 20:22:23 PDT
Created attachment 93860 [details]
Patch
Comment 14 Hajime Morrita 2011-05-17 20:25:26 PDT
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...
Comment 15 Darin Adler 2011-05-18 10:57:00 PDT
Comment on attachment 93860 [details]
Patch

If we change it like this, we should also change it from struct to class.
Comment 16 Tony Chang 2011-05-18 11:27:40 PDT
Comment on attachment 93860 [details]
Patch

Feel free to convert to class in this change or a follow up change.
Comment 17 Hajime Morrita 2011-05-18 18:02:52 PDT
(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.
Comment 18 Hajime Morrita 2011-05-18 18:38:45 PDT
Committed r86813: <http://trac.webkit.org/changeset/86813>
Comment 19 Darin Adler 2011-05-19 09:24:56 PDT
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>.
Comment 20 Hajime Morrita 2011-05-19 20:38:50 PDT
(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...