Bug 98028 - Put implementation details of StyleBuilder.cpp into anonymous namespace
Summary: Put implementation details of StyleBuilder.cpp into anonymous namespace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks: 98005
  Show dependency treegraph
 
Reported: 2012-10-01 06:53 PDT by Yury Semikhatsky
Modified: 2012-10-02 01:11 PDT (History)
18 users (show)

See Also:


Attachments
Patch (12.34 KB, patch)
2012-10-01 07:05 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (12.25 KB, patch)
2012-10-01 07:21 PDT, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-10-01 06:53:13 PDT
Otherwise there may be name collision with the rest of WebCore code. E.g. http://queues.webkit.org/results/14075908 where enum value named Image conflicts with WebCore::Image class.
Comment 1 Yury Semikhatsky 2012-10-01 07:05:23 PDT
Created attachment 166469 [details]
Patch
Comment 2 Pavel Feldman 2012-10-01 07:14:52 PDT
Comment on attachment 166469 [details]
Patch

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

> Source/WebCore/css/StyleBuilder.cpp:896
> +enum BorderImageType { BorderImageImage = 0, BorderImageMask };

BorderImage / BorderMask?
Comment 3 Yury Semikhatsky 2012-10-01 07:20:39 PDT
Comment on attachment 166469 [details]
Patch

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

>> Source/WebCore/css/StyleBuilder.cpp:896
>> +enum BorderImageType { BorderImageImage = 0, BorderImageMask };
> 
> BorderImage / BorderMask?

Done.
Comment 4 Yury Semikhatsky 2012-10-01 07:21:05 PDT
Created attachment 166472 [details]
Patch
Comment 5 Yury Semikhatsky 2012-10-01 07:25:13 PDT
Committed r130047: <http://trac.webkit.org/changeset/130047>
Comment 6 Alexey Proskuryakov 2012-10-01 10:10:44 PDT
We don't normally use anonymous namespaces, primarily because having a function in anonymous namespace often makes debugging harder.

Also, IDEs don't work well when there are name clashes in the project - an IDE doesn't know which "applyInheritValue" you are searching for. So, you need names that are as unique as possible even when using namespaces.

Why did you choose a unique way to solve this issue instead of just using a more descriptive name?
Comment 7 Darin Adler 2012-10-01 14:30:22 PDT
(In reply to comment #6)
> We don't normally use anonymous namespaces, primarily because having a function in anonymous namespace often makes debugging harder.
> 
> Also, IDEs don't work well when there are name clashes in the project - an IDE doesn't know which "applyInheritValue" you are searching for. So, you need names that are as unique as possible even when using namespaces.
> 
> Why did you choose a unique way to solve this issue instead of just using a more descriptive name?

I was about to say the same thing and ask the same question.

We’ve thought about using anonymous namespaces elsewhere in the past, but rejected them for the reasons Alexey cites above.
Comment 8 Yury Semikhatsky 2012-10-02 01:11:13 PDT
(In reply to comment #6)
> We don't normally use anonymous namespaces, primarily because having a function in anonymous namespace often makes debugging harder.
> 
> Also, IDEs don't work well when there are name clashes in the project - an IDE doesn't know which "applyInheritValue" you are searching for. So, you need names that are as unique as possible even when using namespaces.
> 
Didn't know that we try to avoid anonymous namespaces for better debugging experience. I will remove the anonymous namespace as the more specific names introduced in the change don't conflict with other type names in WebCore. Opened a bug on this: wkbug.com/98124

> Why did you choose a unique way to solve this issue instead of just using a more descriptive name?

The names were change as well and that should be enough to resolve original problem. I added anonymous namespace just to avoid such conflicts in the future, will remove it in favor of better debugging and IDE support.