Bug 41832 – [DRT] DRT should be able to dump document markers.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hajime Morrita]

DRT should allow tests to dump document markers.
This is required for testing bugs like Bug 41423 without pixel test.

[Hironori Bono]

Created attachment 61763 [details]
A possible fix

Greetings,

Even though this change is not exactly as you expected, it adds a new JavaScript function 'TextInputController.hasSpellingMarker()' so we can write text-based layout tests for spellchecking. I wish it helps.

Regards,

[Hajime Morrita]

Comment on attachment 61763 [details]
A possible fix

Hi Bono-san, thank you for doing this!

> +
> +/*!
> +    @method hasSpellingMarker
> +    @discussion this function is only for layout tests.
> +    @param location the offset of the range to be checked.
> +    @param length the length of the range to be checked.
> +    @result whether there is a spelling marker in the specified range of the focused node.
> +*/
> +- (BOOL)hasSpellingMarker:(int)location length:(int)length;
If this is for testing purpose, how about to declare this in WebFrameInternal.h ?

[Hironori Bono]

Created attachment 64938 [details]
A possible fix 2

Morita-san,

Thank you for your comment and sorry for my slow update.

> +- (BOOL)hasSpellingMarker:(int)location length:(int)length;
If this is for testing purpose, how about to declare this in WebFrameInternal.h ?

This function is used from TextInputController.m, i.e. outside of WebKit. So, I'm wondering if we can put this function in WebFrameInternal.h. (I need a comment from WebKit experts, though.)

By the way, I have updated my change to make it more compliant with the WebKit coding style. It is definitely helpful to give me your comments.

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 64938 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3256:  Path does not exist. editing/spelling/spelling-contenteditable.html  [test/expectations] [2]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hironori Bono]

Created attachment 65208 [details]
A possible fix 3

Greetings,

Even though the previous change has not been reviewed by nobody, I have updated this change because the previous change does not work any longer since r65787. I wish some one reviews this change before it becomes obsolete.

Regards,

Hironori Bono

[WebKit Review Bot]

Attachment 65208 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3237:  Path does not exist. editing/spelling/spelling-contenteditable.html  [test/expectations] [2]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hironori Bono]

(In reply to comment #6)
> Attachment 65208 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> LayoutTests/platform/chromium/test_expectations.txt:3237:  Path does not exist. editing/spelling/spelling-contenteditable.html  [test/expectations] [2]
> Total errors found: 1 in 16 files
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This is a new test added by my change. I'm wondering why webkit-style complains the file does not exist?

Regards,

Hironori Bono

[Hajime Morrita]

Hi bono-san, thank you for revising the patch and I'm really sorry for my lack of response.

> > +- (BOOL)hasSpellingMarker:(int)location length:(int)length;
> If this is for testing purpose, how about to declare this in WebFrameInternal.h ?
> 
> This function is used from TextInputController.m, i.e. outside of WebKit. So, I'm wondering if we can put this function in WebFrameInternal.h. (I need a comment from WebKit experts, though.)

I'm sorry that I was wrong.
What I meant is WebFramePrivate.h. WebXxxPrivate.h looks the location where
we put test-specific (or other Apple specific) APIs. 
There are WebViewPrivate.h, WebPrefrencesPrivate.h, etc.
WebFrame.h is a part of WebKit public API and will be shipped with WebKit framework.
So it would be safe to avoid something new there.

About a test, how about to adopt script-tests style?
This test is for DumpRenderTree itself and doesn't need to run on the browser.
So we can use script-tests safely and it would make the test more concise.

[Hironori Bono]

Created attachment 65528 [details]
A possible fix 4

(In reply to comment #8)
> I'm sorry that I was wrong.
> What I meant is WebFramePrivate.h. WebXxxPrivate.h looks the location where
> we put test-specific (or other Apple specific) APIs. 
> There are WebViewPrivate.h, WebPrefrencesPrivate.h, etc.
> WebFrame.h is a part of WebKit public API and will be shipped with WebKit framework.
> So it would be safe to avoid something new there.

It makes sense to move this function to WebFramePrivate.h. I have updated my change.
 
> About a test, how about to adopt script-tests style?
> This test is for DumpRenderTree itself and doesn't need to run on the browser.
> So we can use script-tests safely and it would make the test more concise.

These tests depends on "LayoutTests/editing/editing.js" to send editor commands. To move my tests to "LayoutTests/dom/script-tests", I need to copy some functions in the "editing.js" to my tests. I'm wondering if it is good to copy such editor functions to script-tests.

Regards,

[WebKit Review Bot]

Attachment 65528 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3180:  Path does not exist. editing/spelling/spelling-contenteditable.html  [test/expectations] [2]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

> > About a test, how about to adopt script-tests style?
> > This test is for DumpRenderTree itself and doesn't need to run on the browser.
> > So we can use script-tests safely and it would make the test more concise.
> 
> These tests depends on "LayoutTests/editing/editing.js" to send editor commands. To move my tests to "LayoutTests/dom/script-tests", I need to copy some functions in the "editing.js" to my tests. I'm wondering if it is good to copy such editor functions to script-tests.

You can provide TEMPLATE.html which includes both of editing.js and js/resources/js-test-pre.js.
I don't think we have to apply script-tests style in this case.  But I recommend to use js-test-pre.js even if you don't apply scripte-tests style.  It would make the test code simpler.


(In reply to comment #7)
> > LayoutTests/platform/chromium/test_expectations.txt:3237:  Path does not exist. editing/spelling/spelling-contenteditable.html  [test/expectations] [2]
> > Total errors found: 1 in 16 files
> This is a new test added by my change. I'm wondering why webkit-style complains the file does not exist?

Our style checker looks at just diff.  So it doesn't know newly added tests. You can ignore this error.

[Darin Adler]

(In reply to comment #9)
> These tests depends on "LayoutTests/editing/editing.js" to send editor commands. To move my tests to "LayoutTests/dom/script-tests", I need to copy some functions in the "editing.js" to my tests. I'm wondering if it is good to copy such editor functions to script-tests.

Every .js file in script-tests has to be a test; the wrapper-creation process will create a .html for each one of the .js files in that directory. Other .js files should be in other directories and kept out of the script-tests directory.

[Shinichiro Hamaji]

Comment on attachment 65528 [details]
A possible fix 4

WebCore/ChangeLog:9
 +          Tests: editing/spelling/spelling-contentediable.html
contentediable => contentedi*t*able

This might be the reason why the style bot complained. As style bot applies the patch before it checks the patch, a newly added files should be seen from the style checker.

[Hironori Bono]

Created attachment 66187 [details]
A possible fix 5

(In reply to comment #13)
> (From update of attachment 65528 [details])
> WebCore/ChangeLog:9
>  +          Tests: editing/spelling/spelling-contentediable.html
> contentediable => contentedi*t*able
> 
> This might be the reason why the style bot complained. As style bot applies the patch before it checks the patch, a newly added files should be seen from the style checker.

Thank you for noticing this bonehead mistake of mine. I have renamed the test.

Regards,

Hironori Bono

[Kent Tamura]

Comment on attachment 66187 [details]
A possible fix 5

Don't you update the test code?
I meant
 - add <script src="../../fast/js/resource/js-test-pre.js"></script>
 - Use debug(), testPassed(), and testFailed().

[Kent Tamura]

(In reply to comment #15)
>  - add <script src="../../fast/js/resource/js-test-pre.js"></script>

/resource/ -> /resources/

[Hironori Bono]

Created attachment 66478 [details]
A possible fix 6

(In reply to comment #15)
> (From update of attachment 66187 [details])
> Don't you update the test code?
> I meant
>  - add <script src="../../fast/js/resource/js-test-pre.js"></script>
>  - Use debug(), testPassed(), and testFailed().

Sorry, I missed this comment. I have updated my tests to used them.

Regards,

[Kent Tamura]

Comment on attachment 66478 [details]
A possible fix 6

ok, great!

[WebKit Commit Bot]

Comment on attachment 66478 [details]
A possible fix 6

Clearing flags on attachment: 66478

Committed r66721: <http://trac.webkit.org/changeset/66721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hironori Bono]

Greetings,

Thank you all for your review and comments.
By the way, should I file another bug for platform-specific implementation of textInputController.hasSpellingMarker(), such as Chromium?

Regards,

Hironori Bono

[Kent Tamura]

(In reply to comment #21)
> By the way, should I file another bug for platform-specific implementation of textInputController.hasSpellingMarker(), such as Chromium?

I recommend to file another bug.
Discussing multiple patch in one bug is messy.

[Hironori Bono]

(In reply to comment #22)

> I recommend to file another bug.
> Discussing multiple patch in one bug is messy.

Thank you for your advice. I have filed Bug 45441 so I can upload the Chromium implementation of the function.

Regards,

Hironori Bono

[Alexey Proskuryakov]

I think that this should have been in WebFrameInternal indeed, not WebFramePrivate. We didn't want to expose this to code other than our own testing tools.

This has been moved to Internals today, and removing the method from WebFramePrivate.h was a little risky already.