Bug 132863 - Remove BLOB guards
Summary: Remove BLOB guards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-13 05:22 PDT by Tibor Mészáros
Modified: 2014-05-27 07:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (111.47 KB, patch)
2014-05-13 05:38 PDT, Tibor Mészáros
ap: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (512.79 KB, application/zip)
2014-05-13 08:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (475.10 KB, application/zip)
2014-05-13 08:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (516.57 KB, application/zip)
2014-05-13 09:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (571.77 KB, application/zip)
2014-05-13 10:11 PDT, Build Bot
no flags Details
Patch v2 (112.00 KB, patch)
2014-05-13 10:28 PDT, Tibor Mészáros
no flags Details | Formatted Diff | Diff
Patch v4 (122.84 KB, patch)
2014-05-16 08:54 PDT, Tibor Mészáros
no flags Details | Formatted Diff | Diff
Patch (121.51 KB, patch)
2014-05-18 11:58 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch v5 (121.51 KB, patch)
2014-05-19 05:35 PDT, Tibor Mészáros
no flags Details | Formatted Diff | Diff
Patch v6 (115.09 KB, patch)
2014-05-19 07:18 PDT, Tibor Mészáros
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tibor Mészáros 2014-05-13 05:22:05 PDT
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
Comment 1 Tibor Mészáros 2014-05-13 05:38:34 PDT
Created attachment 231371 [details]
Patch

Removed the BLOB guards
Comment 2 Build Bot 2014-05-13 08:01:34 PDT
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
Comment 3 Build Bot 2014-05-13 08:01:39 PDT
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
Comment 4 Build Bot 2014-05-13 08:10:36 PDT
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
Comment 5 Build Bot 2014-05-13 08:10:44 PDT
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
Comment 6 Build Bot 2014-05-13 09:09:35 PDT
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
Comment 7 Build Bot 2014-05-13 09:09:42 PDT
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
Comment 8 Build Bot 2014-05-13 10:11:28 PDT
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
Comment 9 Build Bot 2014-05-13 10:11:37 PDT
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
Comment 10 Alexey Proskuryakov 2014-05-13 10:18:37 PDT
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?
Comment 11 Tibor Mészáros 2014-05-13 10:28:55 PDT
Created attachment 231391 [details]
Patch v2

Fixed patch
Comment 12 Alexey Proskuryakov 2014-05-13 10:44:18 PDT
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.
Comment 13 Csaba Osztrogonác 2014-05-16 03:02:21 PDT
(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.
Comment 14 Tibor Mészáros 2014-05-16 08:54:44 PDT
Created attachment 231575 [details]
Patch v4

Change to alphabetical order in Source/WebCore/WebCore.exp.in, and added symboles to fix win build
Comment 15 Alexey Proskuryakov 2014-05-16 19:33:54 PDT
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.
Comment 16 Csaba Osztrogonác 2014-05-18 11:58:21 PDT
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
Comment 17 Csaba Osztrogonác 2014-05-18 12:55:33 PDT
(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.
Comment 18 Tibor Mészáros 2014-05-19 05:35:48 PDT
Created attachment 231684 [details]
Patch v5
Comment 19 Tibor Mészáros 2014-05-19 07:18:44 PDT
Created attachment 231689 [details]
Patch v6

Patch does not applied, so I updated it.
Comment 20 Csaba Osztrogonác 2014-05-27 06:56:40 PDT
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 )
Comment 21 WebKit Commit Bot 2014-05-27 07:27:52 PDT
Comment on attachment 231689 [details]
Patch v6

Clearing flags on attachment: 231689

Committed r169380: <http://trac.webkit.org/changeset/169380>
Comment 22 WebKit Commit Bot 2014-05-27 07:28:00 PDT
All reviewed patches have been landed.  Closing bug.