Bug 132863 – Remove BLOB guards

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Tibor Mészáros]

It's a feature that everyone has enabled, so we should get rid of the guard?

Reference to a discussion about it:
https://bugs.webkit.org/show_bug.cgi?id=131164

[Tibor Mészáros]

Created attachment 231371 [details]
Patch

Removed the BLOB guards

[Build Bot]

Comment on attachment 231371 [details]
Patch

Attachment 231371 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4880475977940992

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html

[Build Bot]

Created attachment 231380 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231371 [details]
Patch

Attachment 231371 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4991889040211968

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html

[Build Bot]

Created attachment 231381 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231371 [details]
Patch

Attachment 231371 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6587396376231936

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html

[Build Bot]

Created attachment 231385 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231371 [details]
Patch

Attachment 231371 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5267504070918144

New failing tests:
js/dom/global-constructors-attributes-shared-worker.html
js/dom/global-constructors-attributes-dedicated-worker.html

[Build Bot]

Created attachment 231390 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Alexey Proskuryakov]

Comment on attachment 231371 [details]
Patch

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

Windows port doesn't enable blobs. I don't know why, perhaps it should. I'd certainly be super happy to have Blobs enabled unconditionally, it is very annoying and cofusing that some blob related code is enabled, and some is not.

> Source/WebCore/WebCore.exp.in:-2865
> -#if ENABLE(BLOB)
>  __ZN7WebCore12BlobRegistryD2Ev
>  __ZN7WebCore12blobRegistryEv
>  __ZN7WebCore14LoaderStrategy18createBlobRegistryEv
>  __ZTVN7WebCore12BlobRegistryE
>  __ZTVN7WebCore16BlobRegistryImplE
> -#endif

This file is alphabetically sorted, except for conditional exports. Could you please move these lines up to maintain the rule?

[Tibor Mészáros]

Created attachment 231391 [details]
Patch v2

Fixed patch

[Alexey Proskuryakov]

Comment on attachment 231391 [details]
Patch v2

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

> ChangeLog:3
> +        Remove BLOB guards

My comments about the previous version of the patch still apply.

[Csaba Osztrogonác]

(In reply to comment #11)
> Created an attachment (id=231391) [details]
> Patch v2
> Fixed patch
This patch managed to solve the failing test problem.

Please fix the order problem mentioned by Alexey in Comment #10 , and fix the Windows build. You only need to export the necessary symbols with adding them to WebKit/WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in .

I'm not sure why Windows port didn't enable BLOB before, but in this case
I suggest starting a conversation about it on webkit-dev. Or maybe we
should try to enable BLOB on Windows first and later remove the guards
if everything works fine.

[Tibor Mészáros]

Created attachment 231575 [details]
Patch v4

Change to alphabetical order in Source/WebCore/WebCore.exp.in, and added symboles to fix win build

[Alexey Proskuryakov]

I'm confused - doesn't this patch already enable BLOB on Windows by removing the guards? If so, I'll need to find out how to approve it from Apple side.

[Csaba Osztrogonác]

Created attachment 231660 [details]
Patch

Try to make EWS happy. Mac: Add __ZN7WebCore12blobRegistryEv to Source/WebCore/WebCore.exp.in; Windows: Add missing files to Source/WebCore/WebCore.vcxproj/WebCore.vcxproj; revert WebKitExports.def.in changes - I think it isn't necessary, adding missing files to vcxproj file should be enough

[Csaba Osztrogonác]

(In reply to comment #15)
> I'm confused - doesn't this patch already enable BLOB on Windows by removing the guards? If so, I'll need to find out how to approve it from Apple side.

You're right, this patch enables BLOB on Windows with removing the guards,
that's why I mentioned in Comment #13: "maybe we should try to enable BLOB
on Windows first and later remove the guards if everything works fine."

But before doing anything (enable BLOB on Windows / remove guards )": Tibor,
please write a mail about it to webkit-dev to ask the opinion of the wider
WebKit community. Maybe there is a good reason why isn't BLOB enabled on Windows.

[Tibor Mészáros]

Created attachment 231684 [details]
Patch v5

[Tibor Mészáros]

Created attachment 231689 [details]
Patch v6

Patch does not applied, so I updated it.

[Csaba Osztrogonác]

Comment on attachment 231689 [details]
Patch v6

LGTM, r=me.

There weren't any objection on webkit-dev, so let's go ahead and land it.
( https://lists.webkit.org/pipermail/webkit-dev/2014-May/026576.html )

[WebKit Commit Bot]

Comment on attachment 231689 [details]
Patch v6

Clearing flags on attachment: 231689

Committed r169380: <http://trac.webkit.org/changeset/169380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.