Created attachment 154890 [details] Patch

Attachment 154890 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:206: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:209: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 6 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

As discussed on IRC. This doesn't use `document.write`, and I hope appending a text node doesn't fall into the same category. I think it should be possible to extend this to arbitrary text, but this is actually enough for my current usecase. If you think extensibility is important, I'm happy to make that happen. The stylebot is going to complain about `main.cpp`. The conditionals are consistant with their surroundings, even though they don't match the expected style. I'm happy to rewrite the whole block, I suppose, but that should probably be done in a separate patch.

Created attachment 154897 [details] Patch

Attachment 154897 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:206: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:209: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 6 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 154897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154897&action=review > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:207 > + executeScript(obj, "document.body.appendChild(document.createTextNode('PASS!'))"); The problem with this sort of change is that the text node will appear in an unpredictable part of the document because plugin loading is asynchronous. That's similar to the reason document.write doesn't work. Why not just call alert() ?

(In reply to comment #6) > (From update of attachment 154897 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154897&action=review > > > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:207 > > + executeScript(obj, "document.body.appendChild(document.createTextNode('PASS!'))"); > > The problem with this sort of change is that the text node will appear in an unpredictable part of the document because plugin loading is asynchronous. That's similar to the reason document.write doesn't work. Why not just call alert() ? Ideally, I'd like the message to show up with the frame in which it appears. That is, I'd like to know that frame 3 passed, and frame 2 failed. If you look at the example in the first comment of this bug, you'll see the difference. And really, given the way the DRT mock is implemented, I'm not actually sure how asynchronous this code is. I don't think anything's actually loaded. Regardless, it would certainly be possible to include `application/x-webkit-test-netscape,alertonload-Frame 1 passed.` that would alert something with context, but that seems like a bit more effort, and more difficult to automate via `multiple-iframe-test.js`. I'm happy to do it if it's the best option available, however.

> Ideally, I'd like the message to show up with the frame in which it appears. Maybe it should call a log() function in the global scope? Each frame could implement logging and then the message would show up in the right frame.

(In reply to comment #8) > > Ideally, I'd like the message to show up with the frame in which it appears. > > Maybe it should call a log() function in the global scope? Each frame could implement logging and then the message would show up in the right frame. I don't understand how that would address your general concern. The logging function would have to write data to the page somehow, and it seems like we'd end up in the same situation. It would also end up injecting a bit of boilerplate into the test iframe, and it seems like a good idea to avoid that. I think I'll try fiddling with the code to implement a more generic alert mechanism. I think I can make it work with the framework we've already put together for CSP tests, and it's probably better to have a more verbose failure mode anyway. "FAIL: This should have done that other thing instead of loading." is better than "FAIL." :)

> I don't understand how that would address your general concern. The logging function would have to write data to the page somehow, and it seems like we'd end up in the same situation. Typically we create a div with id="console" to log message too. > It would also end up injecting a bit of boilerplate into the test iframe, and it seems like a good idea to avoid that. It woud just be a script tag... > I think I'll try fiddling with the code to implement a more generic alert mechanism. I think I can make it work with the framework we've already put together for CSP tests, and it's probably better to have a more verbose failure mode anyway. "FAIL: This should have done that other thing instead of loading." is better than "FAIL." :) Ok.

Created attachment 155140 [details] Patch

Attachment 155140 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:206: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:214: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #10) > > It would also end up injecting a bit of boilerplate into the test iframe, and it seems like a good idea to avoid that. > > It woud just be a script tag... I see what you mean. That's much simpler than I was making it out to be. I've taken this route. The current implementation adds a `log` attribute to the object, and logs that attribute's contents if the plugin matches `data:application/x-webkit-test-netscape,logifloaded`. That seemed simpler and more readable than trying to extract a substring from the `data:` block. I'm kinda sketchy on the implementation, though. It's been a long time since I couldn't use std::string. Is stack allocation + sprintf kosher here? WDYT?

Created attachment 155142 [details] Reset test results.

Attachment 155142 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:206: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:214: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 155142 [details] Reset test results. Attachment 155142 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13376936

Comment on attachment 155142 [details] Reset test results. Looks good. You might have a windows compile issue to address before landing.

Created attachment 155162 [details] malloc scares me.

Apparently I can't stack allocate on Windows without knowing how large the allocation needs to be (error C2057). `malloc` to the rescue! But it scares me... Assuming it compiles, can you take another look at it to make sure I haven't done something terrible? :)

Comment on attachment 155162 [details] malloc scares me. View in context: https://bugs.webkit.org/attachment.cgi?id=155162&action=review Looks fine, if ugly old skool C. > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:209 > + char* buffer = (char*) malloc(sizeof(char) * (26 + strlen(argv[j]) + 1)); sizeof(char) -> This is always 1 according to the C spec. > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:210 > + sprintf(buffer, "xWebkitTestNetscapeLog('%s')", argv[j]); sprintf -> I would use snprintf out of habit.

Attachment 155162 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:206: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:215: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 155163 [details] snprintf.

(In reply to comment #20) > (From update of attachment 155162 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155162&action=review > > Looks fine, if ugly old skool C. Right. :) I'm assuming there's a good reason this file's old skool? *shrug* > > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:209 > > + char* buffer = (char*) malloc(sizeof(char) * (26 + strlen(argv[j]) + 1)); > > sizeof(char) -> This is always 1 according to the C spec. Dropped. > > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:210 > > + sprintf(buffer, "xWebkitTestNetscapeLog('%s')", argv[j]); > > sprintf -> I would use snprintf out of habit. Good call. Done.

Attachment 155163 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:205: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:206: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:208: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:216: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 155163 [details] snprintf. View in context: https://bugs.webkit.org/attachment.cgi?id=155163&action=review > Tools/DumpRenderTree/TestNetscapePlugIn/main.cpp:211 > + snprintf(buffer, length, "xWebkitTestNetscapeLog('%s')", argv[j]); It doesn't really matter since this is test-only code, but if you're going to use snprintf in production code, you should be aware that it still has a sharp edge: it doesn't guarantee that |buffer| will be null-terminated if you hit the length limit. It's generally a good idiom to write a '\0' into the last byte of the buffer after calling snprintf to make sure that the buffer is null terminated. I wouldn't bother to respin this patch for that, but it's something you might find useful in the future.

Comment on attachment 155163 [details] snprintf. Clearing flags on attachment: 155163 Committed r123978: <http://trac.webkit.org/changeset/123978>

All reviewed patches have been landed. Closing bug.