Re: [webkit-dev] Turn on the PARALLEL_JOBS feature by default
On 10/18/2011 06:42 PM, Darin Adler wrote: I think it’s worth going further — when can we remove this #if altogether? -- Darin Sure we can, sounds great. I will do that in the next patch. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] NeedHelp_In_SpatialNavigation
Hi, Thanks for Replying... Still i have some doubts ...can you help me to clear them. 1) When we use spatial navigation, to find the nearest node it traverse DOM tree, is that node information can not be retrieved from render tree? As per my understanding render tree (after layout) will have the position information for each node. So is there any way we can get the nodes information of any particular area. Please let me know if my understanding is wrong. 2) If from render tree we can get the nodes according to position then we can skip the traversing of whole DOM tree. 3) Is Spatial navigation is related to Hit testing? I have not start profiling it..but soon ill start and share the result. Thanks Sanjeet On Mon, Oct 17, 2011 at 7:33 PM, Antonio Gomes toniki...@gmail.com wrote: Hi. As far as I can tell, the whole DOM is traversed every time a movement is going to happen. I've discussed with Yael briefly on the WebKit summit about some cache mechanisms, but we could not get a nice heuristic to invalidate the cache. Have you profiled it? On Mon, Oct 17, 2011 at 7:29 AM, Sanjeet Pratap Singh sanjeetpratapsi...@gmail.com wrote: Hi All, I am a beginner to WebKit, looking into *Spatial-Navigation* feature of WebKit. I am looking further for some optimisations in the same. I find there *findFocusCandidateInContainer() * in which we start from first child of container and iterate until the we get the focus candidate. My question is,Are we checking the whole DOM tree to find the closest node or we have a small area(container) accordingly? What does the function* container=scrollableEnclosingBoxOrParentFrameForNodeInDirection()* do? Waiting for the reply. Thanks Regards, Sanjeet ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev -- --Antonio Gomes ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] NeedHelp_In_SpatialNavigation
Hi 1) When we use spatial navigation, to find the nearest node it traverse DOM tree, is that node information can not be retrieved from render tree? As per my understanding render tree (after layout) will have the position information for each node. Code tranverses the DOM, yes, but it gets the position information from the associated node renderer. So it gets it from the RenderTree. So is there any way we can get the nodes information of any particular area. Not sure if I understood you here. Could you elaborate? 2) If from render tree we can get the nodes according to position then we can skip the traversing of whole DOM tree. If the RenderTree can tell us what elements in the DOM tree are 'focusable' (which I do not think it can), yes. 3) Is Spatial navigation is related to Hit testing? Not at the moment. -- --Antonio Gomes ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Committing EFL baselines
On 10/17/2011 04:13 PM, Raphael Kubo da Costa wrote: Anyway, are there any options besides simply svn commit'ting all these files? I'm thinking in uploading the text baselines (which are small) first. When these are in place, we could work together to diminish the amount of pixel baselines by adopting things like ScrollbarMock and such. Sounds like a plan? Cheers, Leandro ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up our include discipline
I'd be interested in performance cost to that, especially on a test that creates a lot of JS wrapped Events. -Sam On Oct 18, 2011, at 6:42 PM, Adam Barth wrote: My thought on that is to use a single virtual function and return an atomic string, like we do for elements. Adam On Oct 18, 2011 6:10 PM, Sam Weinig wei...@apple.com wrote: For the specific case of Event, we do need some sort of poor man's RTTI here for the sake of the bindings, but we could consider alternatives. Since you need to identify the subclasses, you need to either use many virtual functions, as we do currently for Event, or have some tagging mechanism, which we do for classes like Element and EventListener. If we use a tagging technique, we need someway to ensure that the tags are unique. In EventListener, we list the types up front in an enum, though this has same downside as the many virtual functions in that the tendrils of WebAudio reach into the base class. For Element, we rely on qualified names, which doesn't require the icky reach, but are slightly more costly. (You still end up with the tendrils in the JS base class). Of course, it might make sense to solve the issue of #ifdefs in Event by simply removing the #ifdefs in Event, since they are just base class implementations. The only downside of that is if we later remove the feature, it is harder to find all the places it touched. -Sam On Oct 18, 2011, at 4:37 PM, Adam Barth wrote: The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Style guide should mention that we don't use anonymous namespace
Hi, This came up again in one of my reviews :( I hate to add a bunch of new rules to style guide but apparently there are 86 instances of anonymous namespaces in WebKit already so it's probably a good idea to mention that we don't normally do this. Best, Ryosuke Niwa Software Engineer Google Inc. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
I'm not an advocate for or against, but it would be nice if we're going to disallow this, that there be a rationale. regards, --Tom Hi, This came up again in one of my reviews :( I hate to add a bunch of new rules to style guide but apparently there are 86 instances of anonymous namespaces in WebKit already so it's probably a good idea to mention that we don't normally do this. Best, Ryosuke Niwa Software Engineer Google Inc. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Tightening up our include discipline
There shouldn't be any performance costs. Instead of returning a boolean (as the virtual function call does today), it will return a constant AtomicString which can be compared to another AtomicString in one instruction. Adam On Wed, Oct 19, 2011 at 1:31 PM, Sam Weinig wei...@apple.com wrote: I'd be interested in performance cost to that, especially on a test that creates a lot of JS wrapped Events. -Sam On Oct 18, 2011, at 6:42 PM, Adam Barth wrote: My thought on that is to use a single virtual function and return an atomic string, like we do for elements. Adam On Oct 18, 2011 6:10 PM, Sam Weinig wei...@apple.com wrote: For the specific case of Event, we do need some sort of poor man's RTTI here for the sake of the bindings, but we could consider alternatives. Since you need to identify the subclasses, you need to either use many virtual functions, as we do currently for Event, or have some tagging mechanism, which we do for classes like Element and EventListener. If we use a tagging technique, we need someway to ensure that the tags are unique. In EventListener, we list the types up front in an enum, though this has same downside as the many virtual functions in that the tendrils of WebAudio reach into the base class. For Element, we rely on qualified names, which doesn't require the icky reach, but are slightly more costly. (You still end up with the tendrils in the JS base class). Of course, it might make sense to solve the issue of #ifdefs in Event by simply removing the #ifdefs in Event, since they are just base class implementations. The only downside of that is if we later remove the feature, it is harder to find all the places it touched. -Sam On Oct 18, 2011, at 4:37 PM, Adam Barth wrote: The other day, Sam chided me on a bug for including a header from WebCore proper in a file in WebCore/platform. Even though I know I'm not supposed to do that, it's an easy mistake to make. There's some work going on in https://bugs.webkit.org/show_bug.cgi?id=49192 to improve the style checker to flag these sorts of bad dependencies. This check isn't live yet, but don't be surprised if some robotic user complains about a bad include in one of your patches. (As always, if the bot complains incorrectly, please let me know so we can eliminate false positives.) Relatedly, as part of the recent ENABLE cleanup effort, I noticed that some files (e.g., http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Event.cpp) attract lots of ifdefs because many unrelated features need to register themselves. I was thinking it might be an improvement to restructure things a bit so that, for example, WebAudio wouldn't need to reach its fingers into Event.cpp and instead could be better contained in the WebCore/webaudio directory. I welcome any thoughts you have on either of these topics. Thanks, Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] insanity of updating 4000+ baseline images due to font rendering change?
We finally have correct baseline images checked in for Chromium-Skia on
SnowLeopard (hooray). Now we face the daunting task of rebaselining for
Chromium-Skia on Leopard.
The problem is that chromium-mac-leopard has ~4500 image diffs compared to
chromium-mac. The vast majority of these diffs are due to very minor
differences in text rendering between Leopard and SnowLeopard. (Details are
in http://code.google.com/p/chromium/issues/detail?id=100904 )
I poked around in LayoutTests/platform (details are at the bottom of this
email) and found that indeed there are 4000-5000 baseline images that
distinguish between Leopard and SnowLeopard. This is presumably why layout
tests generally pass on both platforms.
Before Cary and I spend 2-3 days committing all these new baseline images so
that we can re-enable test_expectations for SnowLeopard... is this the right
approach? It seems pretty inefficient to clog the WebKit repository with
all these baseline images with minor pixel value differences.
Here are the various approaches I can think of... what's the
Hive-Mind-Approved approach?
- Commit 4500 new baseline images for SnowLeopard
- pro: known to work, will catch any regressions that come later
- con: takes a long time to commit, chews up disk space and bandwidth
for all developers, future minor changes may require yet another
set of new
baselines
- Leave all SnowLeopard tests marked as PASS FAIL (or maybe mark them
SKIP) in test_expectations
- pro: known to work, quick and easy, doesn't clog repo space and
developer update bandwidth, future minor changes won't break any bots
- con: will not catch any regressions that come later on SnowLeopard
- Remove descriptive text from all these tests, so that text rendering is
only evaluated in tests specifically for that purpose
- pro: prevents this problem for future OS versions, should allow for
lots more baseline images to be shared across platforms
- con: a lot of work to replace all existing baseline images, must
coordinate across community of Chromium/WebKit developers, tests will be
more difficult to interpret without text
- Figure out how our test pages can be rendered with a completely
cross-platform pixel-equivalent font
- pro: similar to above but tests keep their descriptive text
- con: similar to above but more technically challenging
- Augment our pixel-diff tools to allow for comparison masks (only pay
attention to pixel diffs within this rectangle)
- pro: existing baseline images can stay in place, and perhaps be
shared with new OS versions and platforms
- con: requires modification of pixel-diff tools, need to add
comparison mask to each test definition
Details on how I determined that there are already 4000-5000 baseline images
that distinguish between Leopard and SnowLeopard for existing platforms
(Safari and chromium-cg):
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ mkdir tests
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ find
platform -name *.png tests/all-pngs
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ wc -l
tests/all-pngs
45529 tests/all-pngs
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ cat
tests/all-pngs | awk -F '/' '{print $2}' | sort | uniq
tests/list-all-platforms
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ grep mac
tests/list-all-platforms tests/list-mac-platforms
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ mkdir
tests/by-platform
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ for PLATFORM
in $(cat tests/list-mac-platforms); do grep ^platform/$PLATFORM/
tests/all-pngs | sed s/platform\\/$PLATFORM\\///
tests/by-platform/$PLATFORM ; done
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ wc -l
tests/by-platform/*
1050 tests/by-platform/chromium-cg-mac
1575 tests/by-platform/chromium-cg-mac-leopard
114 tests/by-platform/chromium-cg-mac-snowleopard
114 tests/by-platform/chromium-gpu-cg-mac
135 tests/by-platform/chromium-gpu-mac
4909 tests/by-platform/chromium-mac
79 tests/by-platform/chromium-mac-leopard
131 tests/by-platform/chromium-mac-snowleopard
7631 tests/by-platform/mac
4435 tests/by-platform/mac-leopard
247 tests/by-platform/mac-snowleopard
7 tests/by-platform/mac-wk2
20427 total
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ cat
tests/by-platform/chromium-cg-mac
tests/by-platform/chromium-cg-mac-snowleopard tests/by-platform/mac
tests/by-platform/mac-snowleopard | sort | uniq
tests/list-snowleopard-cg-images
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ cat
tests/by-platform/chromium-cg-mac-leopard
tests/by-platform/mac-leopard | sort | uniq
tests/list-leopard-cg-images
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ cat
tests/list-leopard-cg-images tests/list-snowleopard-cg-images | sort |
uniq
Re: [webkit-dev] insanity of updating 4000+ baseline images due to font rendering change?
I definitely agree that our current approach to pixel baselines is
less than ideal. There have been a number of efforts to reduce the
number of pixel tests, for example by converting tests to text tests
or to ref tests, but even after those efforts, we still have thousands
of tests that require different baselines on essentially every
platform and port.
We'd prefer not to leave the Chromium Leopard test expectations has
PASS FAIL in the long term because that removes a large amount of
test coverage (~20%).
How confident are you that flipping the CG - Skia switch will stick this time?
Once we're sure the switch will stay in the Skia position, we can
delete the Chromium Leopard CG expected results, which will leave the
net quantity of results roughly the same. Also, once you're confident
in the Leopard Skia results, there shouldn't be too much churn (e.g.,
as there was when fixing the focus ring and the tiny font rendering).
Adam
On Wed, Oct 19, 2011 at 2:04 PM, Elliot Poger epo...@google.com wrote:
We finally have correct baseline images checked in for Chromium-Skia on
SnowLeopard (hooray). Now we face the daunting task of rebaselining for
Chromium-Skia on Leopard.
The problem is that chromium-mac-leopard has ~4500 image diffs compared to
chromium-mac. The vast majority of these diffs are due to very minor
differences in text rendering between Leopard and SnowLeopard. (Details are
in http://code.google.com/p/chromium/issues/detail?id=100904 )
I poked around in LayoutTests/platform (details are at the bottom of this
email) and found that indeed there are 4000-5000 baseline images that
distinguish between Leopard and SnowLeopard. This is presumably why layout
tests generally pass on both platforms.
Before Cary and I spend 2-3 days committing all these new baseline images so
that we can re-enable test_expectations for SnowLeopard... is this the right
approach? It seems pretty inefficient to clog the WebKit repository with
all these baseline images with minor pixel value differences.
Here are the various approaches I can think of... what's the
Hive-Mind-Approved approach?
Commit 4500 new baseline images for SnowLeopard
pro: known to work, will catch any regressions that come later
con: takes a long time to commit, chews up disk space and bandwidth for all
developers, future minor changes may require yet another set of new
baselines
Leave all SnowLeopard tests marked as PASS FAIL (or maybe mark them
SKIP) in test_expectations
pro: known to work, quick and easy, doesn't clog repo space and developer
update bandwidth, future minor changes won't break any bots
con: will not catch any regressions that come later on SnowLeopard
Remove descriptive text from all these tests, so that text rendering is only
evaluated in tests specifically for that purpose
pro: prevents this problem for future OS versions, should allow for lots
more baseline images to be shared across platforms
con: a lot of work to replace all existing baseline images, must coordinate
across community of Chromium/WebKit developers, tests will be more difficult
to interpret without text
Figure out how our test pages can be rendered with a completely
cross-platform pixel-equivalent font
pro: similar to above but tests keep their descriptive text
con: similar to above but more technically challenging
Augment our pixel-diff tools to allow for comparison masks (only pay
attention to pixel diffs within this rectangle)
pro: existing baseline images can stay in place, and perhaps be shared with
new OS versions and platforms
con: requires modification of pixel-diff tools, need to add comparison mask
to each test definition
Details on how I determined that there are already 4000-5000 baseline images
that distinguish between Leopard and SnowLeopard for existing platforms
(Safari and chromium-cg):
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ mkdir tests
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ find
platform -name *.png tests/all-pngs
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ wc -l
tests/all-pngs
45529 tests/all-pngs
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ cat
tests/all-pngs | awk -F '/' '{print $2}' | sort | uniq
tests/list-all-platforms
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ grep mac
tests/list-all-platforms tests/list-mac-platforms
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ mkdir
tests/by-platform
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ for PLATFORM
in $(cat tests/list-mac-platforms); do grep ^platform/$PLATFORM/
tests/all-pngs | sed s/platform\\/$PLATFORM\\///
tests/by-platform/$PLATFORM ; done
~/src/webkit/rebaseline-greenify-bots/WebKit/LayoutTests$ wc -l
tests/by-platform/*
1050 tests/by-platform/chromium-cg-mac
1575 tests/by-platform/chromium-cg-mac-leopard
114 tests/by-platform/chromium-cg-mac-snowleopard
114
[webkit-dev] watchlist: A tool to alert you about patches of interest.
The watchlist is a simple way to watch for new patches that interest you. The
watchlist is automatically applied to patches by a bot (currently the style
bot).
I'm happy to answer questions about it here or in irc (and/or review any
patches you make to the config file, but of course I don't mind others
reviewing those patches or answering questions either).
Here the details on how to use it from
https://wiki.webkit.org/wiki/WatchList
How to use the watch list
https://wiki.webkit.org/wiki/WatchList#Howtousethewatchlist
You’ll need to create a definition which matches patches that you are
interested in or find one that already exists. You’ll need to add a rule to
cc yourself on the bug (or add a message to the bug).
Details
https://wiki.webkit.org/wiki/WatchList#Details
The watchlist file is here:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlisthttp://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist
Here’s an example version:
{
DEFINITIONS: {
ThreadingFiles: {
filename: rSource/JavaScriptCore/wtf/ThreadSpecific\.
r|Source/JavaScriptCore/wtf/ThreadSafeRefCounted\.
},
ThreadingUsage: {
more: r(CrossThreadCopier|CrossThreadRefCounted)(?!\.(h|cpp)),
},
},
CC_RULES: {
ThreadingFiles|ThreadingUsage: [ levin+thread...@chromium.org, ],
},
MESSAGE_RULES: {
ThreadingUsage: [ Are you sure you want to using threading?!?, ],
},
}
Definitions section
https://wiki.webkit.org/wiki/WatchList#Definitionssection
The definitions section is where you define what you want to look for. If it
is a filename pattern, use “filename”. Filename matches are a prefix match.
If is a code pattern use “more” or “less”, if you want to know if more or
less of instances in that pattern occur. (more use the regex to find a match
modified lines in a patch. Then it searches the to see if more instances of
that exact text occur on a per file basis.)
A definition is said to match if all of its clauses are true for any file in
a patch. If you could look for more instances of a pattern occurring only
within a group of a files by using both “filename” and “more” together.
CC rules
https://wiki.webkit.org/wiki/WatchList#CCrules
The cc rules section is where you list who should be added when a definition
matches. You can or together definitions but I only recommend doing this
when the definitions are highly related.
Message rules
https://wiki.webkit.org/wiki/WatchList#Messagerules
The message rules is where you list any messages that should be added to a
bug when a definition matches.
Trying out your change
https://wiki.webkit.org/wiki/WatchList#Tryingoutyourchange
Do a change locally that should trigger your rule and run: webkit-patch
apply-watchlist-local
It should tell you who would be cc’ed and any messages that would be added
to the bug.
Check your change for mistakes
https://wiki.webkit.org/wiki/WatchList#Checkyourchangeformistakes
Mistakes will slow things down or mess up your change. Run
check-webkit-style on your patch to catch many of these errors.
Appendix: Details about the regex used in the example:
https://wiki.webkit.org/wiki/WatchList#Appendix:Detailsabouttheregexusedintheexample:
One twist in the “more” match is the ?!\.(h|cpp)). This prevents matching
mentions of CrossThreadRefCounted.h due to includes, build files, etc. which
I don’t care about.
The r is a python thing which means that the \ in the string don’t escape
characters (r”\n” is r“\” +”n”) which is handy when you need the \ to escape
regex characters.
Python’s regex format is documented here:
http://docs.python.org/library/re.htmlhttp://docs.python.org/library/re.html
Best wishes,
dave
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
The guideline is not to disallow anonymous namespaces. It’s to prefer “static” over anonymous namespaces when either one would work. Debugging tools on at least some of the platforms work better with functions that are given internal linkage by using the “static” keyword rather than functions that are inside anonymous namespaces. On the other hand, anonymous namespaces are a more powerful tool that can do more than the “static” keyword can. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? - Ryosuke On Wed, Oct 19, 2011 at 3:45 PM, Darin Adler da...@apple.com wrote: The guideline is not to disallow anonymous namespaces. It’s to prefer “static” over anonymous namespaces when either one would work. Debugging tools on at least some of the platforms work better with functions that are given internal linkage by using the “static” keyword rather than functions that are inside anonymous namespaces. On the other hand, anonymous namespaces are a more powerful tool that can do more than the “static” keyword can. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
I'm interested in learning the existing convention, not reasons to prefer one or another. - Ryosuke On Wed, Oct 19, 2011 at 3:50 PM, John Knottenbelt jknot...@chromium.orgwrote: I would recommend wrapping such classes in an anonymous namespace to avoid surprising link errors due to unintentional name collision. Such problems can also be difficult to spot at first as sometimes the linker just works and then you get a seg fault sometime later. On Wed, Oct 19, 2011 at 3:47 PM, Ryosuke Niwa rn...@webkit.org wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? - Ryosuke On Wed, Oct 19, 2011 at 3:45 PM, Darin Adler da...@apple.com wrote: The guideline is not to disallow anonymous namespaces. It’s to prefer “static” over anonymous namespaces when either one would work. Debugging tools on at least some of the platforms work better with functions that are given internal linkage by using the “static” keyword rather than functions that are inside anonymous namespaces. On the other hand, anonymous namespaces are a more powerful tool that can do more than the “static” keyword can. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Oct 19, 2011, at 3:47 PM, Ryosuke Niwa wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Can anyone name a single time in the entire history of the WebKit project where we had a problem with class name collision? -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] watchlist: A tool to alert you about patches of interest.
I've been experimenting with watchlists as Dave has been working on
them, and I find them very helpful. For example, if you'd like to be
CCed whenever someone changes a web-facing IDL file, you subscribe to
WebIDL changes, as I have:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist#L75
Adam
On Wed, Oct 19, 2011 at 3:40 PM, David Levin le...@chromium.org wrote:
The watchlist is a simple way to watch for new patches that interest
you. The watchlist is automatically applied to patches by a bot (currently
the style bot).
I'm happy to answer questions about it here or in irc (and/or review any
patches you make to the config file, but of course I don't mind others
reviewing those patches or answering questions either).
Here the details on how to use it
from https://wiki.webkit.org/wiki/WatchList
How to use the watch list
You’ll need to create a definition which matches patches that you are
interested in or find one that already exists. You’ll need to add a rule to
cc yourself on the bug (or add a message to the bug).
Details
The watchlist file is
here: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist
Here’s an example version:
{
DEFINITIONS: {
ThreadingFiles: {
filename: rSource/JavaScriptCore/wtf/ThreadSpecific\.
r|Source/JavaScriptCore/wtf/ThreadSafeRefCounted\.
},
ThreadingUsage: {
more:
r(CrossThreadCopier|CrossThreadRefCounted)(?!\.(h|cpp)),
},
},
CC_RULES: {
ThreadingFiles|ThreadingUsage: [ levin+thread...@chromium.org,
],
},
MESSAGE_RULES: {
ThreadingUsage: [ Are you sure you want to using threading?!?,
],
},
}
Definitions section
The definitions section is where you define what you want to look for. If it
is a filename pattern, use “filename”. Filename matches are a prefix match.
If is a code pattern use “more” or “less”, if you want to know if more or
less of instances in that pattern occur. (more use the regex to find a match
modified lines in a patch. Then it searches the to see if more instances of
that exact text occur on a per file basis.)
A definition is said to match if all of its clauses are true for any file in
a patch. If you could look for more instances of a pattern occurring only
within a group of a files by using both “filename” and “more” together.
CC rules
The cc rules section is where you list who should be added when a definition
matches. You can or together definitions but I only recommend doing this
when the definitions are highly related.
Message rules
The message rules is where you list any messages that should be added to a
bug when a definition matches.
Trying out your change
Do a change locally that should trigger your rule and run: webkit-patch
apply-watchlist-local
It should tell you who would be cc’ed and any messages that would be added
to the bug.
Check your change for mistakes
Mistakes will slow things down or mess up your change.
Run check-webkit-style on your patch to catch many of these errors.
Appendix: Details about the regex used in the example:
One twist in the “more” match is the ?!\.(h|cpp)). This prevents matching
mentions of CrossThreadRefCounted.h due to includes, build files, etc. which
I don’t care about.
The r is a python thing which means that the \ in the string don’t escape
characters (r”\n” is r“\” +”n”) which is handy when you need the \ to escape
regex characters.
Python’s regex format is documented
here: http://docs.python.org/library/re.html
Best wishes,
dave
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
I’m open to changing our style. The compiler many of us used in the past, gcc, had problems generating correct debug information for code in anonymous namespaces. Given that most uses didn’t accomplish anything more than putting static in front of functions did, we decided to stick with that. But that old gcc problem is not an active problem for most WebKit developers, since we’re almost all using either clang or Visual C++. As far as classes are concerned, I am not sure what kinds of difficulties qw might have debugging if there was no simple way to type a class’s name unambiguously. I’m sure that at least some things that are easier if each class has a unique name. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Wed, Oct 19, 2011 at 3:51 PM, Darin Adler da...@apple.com wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Has this been a convention we use? - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Oct 19, 2011, at 3:58 PM, Ryosuke Niwa wrote: On Wed, Oct 19, 2011 at 3:51 PM, Darin Adler da...@apple.com wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Has this been a convention we use? I’m not sure how to answer that. I have not used anonymous namespaces in any WebKit code I contributed, and the first case of doing so that I recall was when Adam Barth used some in the HTML parser work. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Wed, Oct 19, 2011 at 4:00 PM, Darin Adler da...@apple.com wrote: On Oct 19, 2011, at 3:58 PM, Ryosuke Niwa wrote: On Wed, Oct 19, 2011 at 3:51 PM, Darin Adler da...@apple.com wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Has this been a convention we use? I’m not sure how to answer that. I have not used anonymous namespaces in any WebKit code I contributed, and the first case of doing so that I recall was when Adam Barth used some in the HTML parser work. Okay. Sounds like it's left to reviewers' and committers' discretions. It'll be still nice to give some guidance on when we should and should not use anonymous namespaces so that I don't have to engage in a debate on every code review. I personally don't like anonymous namespaces because VS.net's class view separates classes within anonymous namespace from the rest: http://goo.gl/2IkzQ (screen shot) But I think we've established that we prefer having unique class/function names over using anonymous namespaces. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Wed, Oct 19, 2011 at 4:29 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Oct 19, 2011 at 4:00 PM, Darin Adler da...@apple.com wrote: On Oct 19, 2011, at 3:58 PM, Ryosuke Niwa wrote: On Wed, Oct 19, 2011 at 3:51 PM, Darin Adler da...@apple.com wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Has this been a convention we use? I’m not sure how to answer that. I have not used anonymous namespaces in any WebKit code I contributed, and the first case of doing so that I recall was when Adam Barth used some in the HTML parser work. Okay. Sounds like it's left to reviewers' and committers' discretions. It'll be still nice to give some guidance on when we should and should not use anonymous namespaces so that I don't have to engage in a debate on every code review. I personally don't like anonymous namespaces because VS.net's class view separates classes within anonymous namespace from the rest: http://goo.gl/2IkzQ (screen shot) But I think we've established that we prefer having unique class/function names over using anonymous namespaces. Note that unless we're running some extra analysis over the codebase beyond the standard compilation, uniqueness won't be enforced. The compiler isn't required to warn if the same name is used in different translation units. While this might be only a theoretical danger (Darin asked above for cases where we've actually had name collisions, and I haven't been around long enough to know the answer), the symptoms of such a collision can be quite subtle. - Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
Another benefit to using the static keyword is that you can tell at a glance that the function is internal to the translation unit, while you may not notice the anonymous namespace. -Sam On Oct 19, 2011, at 3:45 PM, Darin Adler wrote: The guideline is not to disallow anonymous namespaces. It’s to prefer “static” over anonymous namespaces when either one would work. Debugging tools on at least some of the platforms work better with functions that are given internal linkage by using the “static” keyword rather than functions that are inside anonymous namespaces. On the other hand, anonymous namespaces are a more powerful tool that can do more than the “static” keyword can. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
Also... VC++'s debugger happens to have some issues around unnamed namespaces: http://msdn.microsoft.com/en-us/library/0888kc6a%28VS.80%29.aspx Don't recall if this affected classes inside unnamed namespace though. I think VC++ 2005's support for unnamed namespace is better than nested classes (cl.exe has some serious bugs in resolving nested class names) so if our only alternative to unnamed namespace is nested classes, then I'd much prefer unnamed namespace. Adam, do you recall any compiler/debugger issues with classes inside unnamed namespace on VC++ 2005? - Ryosuke On Wed, Oct 19, 2011 at 4:29 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Oct 19, 2011 at 4:00 PM, Darin Adler da...@apple.com wrote: On Oct 19, 2011, at 3:58 PM, Ryosuke Niwa wrote: On Wed, Oct 19, 2011 at 3:51 PM, Darin Adler da...@apple.com wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Has this been a convention we use? I’m not sure how to answer that. I have not used anonymous namespaces in any WebKit code I contributed, and the first case of doing so that I recall was when Adam Barth used some in the HTML parser work. Okay. Sounds like it's left to reviewers' and committers' discretions. It'll be still nice to give some guidance on when we should and should not use anonymous namespaces so that I don't have to engage in a debate on every code review. I personally don't like anonymous namespaces because VS.net's class view separates classes within anonymous namespace from the rest: http://goo.gl/2IkzQ (screen shot) But I think we've established that we prefer having unique class/function names over using anonymous namespaces. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Wed, Oct 19, 2011 at 6:24 PM, Ryosuke Niwa rn...@webkit.org wrote: Also... VC++'s debugger happens to have some issues around unnamed namespaces: http://msdn.microsoft.com/en-us/library/0888kc6a%28VS.80%29.aspx Don't recall if this affected classes inside unnamed namespace though. I think VC++ 2005's support for unnamed namespace is better than nested classes (cl.exe has some serious bugs in resolving nested class names) so if our only alternative to unnamed namespace is nested classes, then I'd much prefer unnamed namespace. Adam, do you recall any compiler/debugger issues with classes inside unnamed namespace on VC++ 2005? Nope, but I haven't used VC 2005 in many years. Adam On Wed, Oct 19, 2011 at 4:29 PM, Ryosuke Niwa rn...@webkit.org wrote: On Wed, Oct 19, 2011 at 4:00 PM, Darin Adler da...@apple.com wrote: On Oct 19, 2011, at 3:58 PM, Ryosuke Niwa wrote: On Wed, Oct 19, 2011 at 3:51 PM, Darin Adler da...@apple.com wrote: How about classes that are only used in one cpp file? Should we be wrapping those in an anonymous namespace? I’d suggest not wrapping them in an anonymous namespace. Debugging tools work better when classes have unique names. Has this been a convention we use? I’m not sure how to answer that. I have not used anonymous namespaces in any WebKit code I contributed, and the first case of doing so that I recall was when Adam Barth used some in the HTML parser work. Okay. Sounds like it's left to reviewers' and committers' discretions. It'll be still nice to give some guidance on when we should and should not use anonymous namespaces so that I don't have to engage in a debate on every code review. I personally don't like anonymous namespaces because VS.net's class view separates classes within anonymous namespace from the rest: http://goo.gl/2IkzQ (screen shot) But I think we've established that we prefer having unique class/function names over using anonymous namespaces. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] reminder: don't do blanket style fixups
I saw a patch get committed recently that just fixes trailing whitespace across a large swath of the codebase. In general, we prefer that pure style cleanups are only done as a precursor to actually modifying the code. Otherwise, while they serve to make the style consistent, they also make it considerably harder to dig through the commit history for a given chunk of code. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Style guide should mention that we don't use anonymous namespace
On Wed, Oct 19, 2011 at 6:32 PM, Adam Barth aba...@webkit.org wrote: On Wed, Oct 19, 2011 at 6:24 PM, Ryosuke Niwa rn...@webkit.org wrote: Also... VC++'s debugger happens to have some issues around unnamed namespaces: http://msdn.microsoft.com/en-us/library/0888kc6a%28VS.80%29.aspx Don't recall if this affected classes inside unnamed namespace though. I think VC++ 2005's support for unnamed namespace is better than nested classes (cl.exe has some serious bugs in resolving nested class names) so if our only alternative to unnamed namespace is nested classes, then I'd much prefer unnamed namespace. Adam, do you recall any compiler/debugger issues with classes inside unnamed namespace on VC++ 2005? Nope, but I haven't used VC 2005 in many years. Oops, sorry I meant to cc A. Roben. Adam (R), do you remember any issues with unnamed namespace? - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] reminder: don't do blanket style fixups
Trailing whitespace guidelines aren't part of the WebKit style guide on purpose. Thus, it wasn't a style clean-up, so I guess that would make it a change to fit someone's preference. dave On Wed, Oct 19, 2011 at 6:38 PM, Ojan Vafai o...@chromium.org wrote: I saw a patch get committed recently that just fixes trailing whitespace across a large swath of the codebase. In general, we prefer that pure style cleanups are only done as a precursor to actually modifying the code. Otherwise, while they serve to make the style consistent, they also make it considerably harder to dig through the commit history for a given chunk of code. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] NRWT Migration Update
After a long hiatus, I'm back working on NRWT. As of this evening all of the blocking issues to switching the WK2 bot are resolved: https://bugs.webkit.org/show_bug.cgi?id=56729 Just waiting for the WK2 bot to have some smaller number of failures, then I'll pull the trigger. After WK2 is switched to NRWT, there is only --leaks and Windows to switch, before we can consider deleting ORWT. Interested parties can continue to track the NRWT migration here: https://bugs.webkit.org/show_bug.cgi?id=34984 -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
