Bug 114950 - DOMFileSystemBase: fix multiple definitions in the BlackBerry port
Summary: DOMFileSystemBase: fix multiple definitions in the BlackBerry port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 111729
  Show dependency treegraph
 
Reported: 2013-04-22 05:17 PDT by Alberto Garcia
Modified: 2013-04-29 13:03 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.49 KB, patch)
2013-04-22 05:21 PDT, Alberto Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 2013-04-22 05:17:51 PDT
The following  methods are already defined in DOMFileSystemBlackBerry.cpp:

crackFileSystemURL()
createFileSystemURL()
isValidType()
supportsToURL()
Comment 1 Alberto Garcia 2013-04-22 05:21:12 PDT
Created attachment 199017 [details]
Patch
Comment 2 Xan Lopez 2013-04-24 06:58:27 PDT
Comment on attachment 199017 [details]
Patch

OK.
Comment 3 WebKit Commit Bot 2013-04-24 07:26:05 PDT
Comment on attachment 199017 [details]
Patch

Clearing flags on attachment: 199017

Committed r149034: <http://trac.webkit.org/changeset/149034>
Comment 4 WebKit Commit Bot 2013-04-24 07:26:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Benjamin Poulain 2013-04-28 21:04:15 PDT
What The Fuck?

This should have:
-Justification on why you did not make an abstraction instead.
-Justification for using different implementation.

Adding platform #ifdef anywhere at random places in WebCore is not okay, we are trying to remove them. Please explain.
Comment 6 Xan Lopez 2013-04-29 00:29:01 PDT
(In reply to comment #5)
> What The Fuck?
> 
> This should have:
> -Justification on why you did not make an abstraction instead.
> -Justification for using different implementation.
> 
> Adding platform #ifdef anywhere at random places in WebCore is not okay, we are trying to remove them. Please explain.

The file already had #ifdefs for Chromium, which were removed recently with the removal of the Chromium code. It just so happens that the BB port was also using this feature, but the #ifdefs were not upstreamed. So the justification is that in general adding an extra #ifdef when there's already one is not a big deal, but we can open another bug to improve the code.
Comment 7 Alberto Garcia 2013-04-29 03:15:14 PDT
(In reply to comment #5)
> -Justification for using different implementation.

Apart from what Xan said, I'll try to see if we can merge both
implementations now that there's no Chromium code around.
Comment 8 Benjamin Poulain 2013-04-29 13:03:56 PDT
This has landed last week, there is no chromium code anymore.
Even when chromium was there, they were doing a ton more maintenance than 
(In reply to comment #7)
> (In reply to comment #5)
> > -Justification for using different implementation.
> 
> Apart from what Xan said, I'll try to see if we can merge both
> implementations now that there's no Chromium code around.

Thanks. That is the right thing to do.