Re: [webkit-dev] Do we have a style preference about const member functions?
On May 31, 2011, at 10:00 PM, Brent Fulgham wrote:
On May 31, 2011, at 8:44 PM, Maciej Stachowiak wrote:
For example, the compiler does not tell you that the following
implementation of Node::previousSibling() (currently in our code!) is
totally wrong from the logical const perspective:
Node* previousSibling() const { return m_previous; }
The correct way to do const-correctness here would require more verbosity:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
And the first variant would be dead code if we don't ever actually use const
node pointers.
Given your views on const-correctness discipline, what do you think is the
right way to handle this situation? Note that this pattern comes up for many
core DOM methods and many render tree methods.
One possible (though ugly) way of allowing the compiler to do some of this
work for you is to declare the m_previous member as a const pointer, and then
manipulate it only in specific routines using the const_cast operator to
allow it to mutate. But this is probably a case of the cure being worse than
the disease.
This cure would most definitely be worse than the disease. All JavaScript
access to the DOM gets (effectively) non-const node references, so we'd need to
cast away const everywhere.
If we had logic that iterated over the node siblings in read-only fashion, we
would ideally do so through a const iterator. In addition to documenting
that we don't intend to mutate the object, it would provide potentially
useful information that compilers could use to make aliasing decisions and so
forth. Perhaps we never iterate over DOM nodes without intending to mutate
them; if so, I would agree that there is no benefit to maintaining the const
variation.
It's my understanding that compilers make essentially no use of a method being
labeled const to enable optimizations. Given const_cast and mutable, just
knowing the signature is const is not a strong enough guarantee to decide
anything at the call site. And if the compilercan see the body of the function,
and see that it actually doesn't mutate the object, it know all it needs to
know and are not helped by the const declaration. Marking methods as const to
help the compiler is, as far as I know, simply cargo cult engineering.
However I do not think (as the Node example might imply) that the fact that
the compiler cannot identify ALL categories of const error means that we are
better off washing our hands of const correctness.
Let's set aside the compiler's error checking for the moment. What do you
believe is the proper const-correct way to write previousSibling() and related
methods? A priori, I think the correct way is this:
Node* previousSibling() { return m_previous; }
I could also be convinced that the following is technically more correct,
though in a way that is completely useless for our code base at present:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
The following would be valid but would require us to cast away const all over
the place and would therefore in my opinion be a net negative:
const Node* previousSibling() const { return m_previous; }
And what's in our code now violates the intent of const, and so to me at least
is clearly a bug:
Node* previousSibling() const { return m_previous; }
What do you think is the right way to do it? One of these? Something else?
In fact, due to the viral nature of const-ness changes, leaving them in (and
continuing to maintain them) is a good long term approach since the first
time you want to work with a const Node object you will have to resurrect
const variations of methods across the object hierarchy.
Well one big problem right now (just from scanning the core DOM classes) is
that we have a lot of clearly broken use of const. We could (a) leave it as-is,
(b) remove the incorrect use of const, or (c) deploy proper const-correctness.
it seems like you are against (b), but I cannot tell if you advocate (a) or (c).
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Issue Analysis:48290 [HTML Canvas globalCompositeOperation]
Hi All, I was going through the bug https://bugs.webkit.org/show_bug.cgi?id=48290, which passes is FF but fails in Safari/Winlauncher( r87771). As per http://www.w3.org/TR/2011/WD-2dcontext-20110525/#compositing, highlight is NOT a valid composite operation. but it is being mentioned in platform/graphics/GraphicsTypes.cpp in compositeOperatorNames[] that's why this issue is present. When I removed highlight from here also removed CompositeHighLight from enum CompositeOperator in GraphicsType.h this issue is resolved (I also had to comment out all other code using CompositeHighLight CSSValueHighlight ). However, the following comment was mentioned in GraphicsType.h (// Note: These constants exactly match the NSCompositeOperator constants of // AppKit on Mac OS X Tiger. If these ever change, we'll need to change the // Mac OS X Tiger platform code to map one to the other. ). So I am little unclear what is the purpose of this CompositeHighLight. Can someone please help me out here. Thanks Regards, Karthik ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Issue Analysis:48290 [HTML Canvas globalCompositeOperation]
On May 31, 2011, at 11:49 PM, Karthik wrote: However, the following comment was mentioned in GraphicsType.h (// Note: These constants exactly match the NSCompositeOperator constants of // AppKit on Mac OS X Tiger. If these ever change, we'll need to change the // Mac OS X Tiger platform code to map one to the other. ). So I am little unclear what is the purpose of this CompositeHighLight. We can remove that comment. It’s obsolete. I believe the “highlight” compositing mode used to be supported in canvas by WebKit on Mac, but hasn’t been for some time. There’s probably no harm in removing it now. The fact that the string is “not valid” in the specification isn’t what makes it OK to remove. Please remember that the canvas implementation in WebKit was the first and predates the specification by at least a year. But it’s almost certainly OK to remove this compositing mode because it hasn’t been implemented for quite a while. There’s still a slim chance that there is some content out there relying on “highlight” being a synonym for “source-over”, but it’s not likely. One side comment: I think the mapping from strings to the various graphics enums really belongs in the canvas code, not in the platform directory. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Issue Analysis:48290 [HTML Canvas globalCompositeOperation]
Hi Darin, Thanks for your detailed explanation. Initially we decided to remove the string as well the enum to fix this issue. But Karthik's patch (we have already submitted the patch in https://bugs.webkit.org/show_bug.cgi?id=48290) caused a Chromium build issue then we found that this enum is used in lot of other places. So, we were a little apprehensive about the side effect of this fix. Therefore, we submitted another patch where we are returning from the canvas code itself if the compositing mode is highlight. We are still waiting for the review comments. Similar issue (https://bugs.webkit.org/show_bug.cgi?id=48289 ) we have also seen for darker/CompositePlusDarker (not a valid compositing mode any more). Can we also remove this as well? Thanks, Rahaman On Wed, Jun 1, 2011 at 9:51 PM, Darin Adler da...@apple.com wrote: On May 31, 2011, at 11:49 PM, Karthik wrote: However, the following comment was mentioned in GraphicsType.h (// Note: These constants exactly match the NSCompositeOperator constants of // AppKit on Mac OS X Tiger. If these ever change, we'll need to change the // Mac OS X Tiger platform code to map one to the other. ). So I am little unclear what is the purpose of this CompositeHighLight. We can remove that comment. It’s obsolete. I believe the “highlight” compositing mode used to be supported in canvas by WebKit on Mac, but hasn’t been for some time. There’s probably no harm in removing it now. The fact that the string is “not valid” in the specification isn’t what makes it OK to remove. Please remember that the canvas implementation in WebKit was the first and predates the specification by at least a year. But it’s almost certainly OK to remove this compositing mode because it hasn’t been implemented for quite a while. There’s still a slim chance that there is some content out there relying on “highlight” being a synonym for “source-over”, but it’s not likely. One side comment: I think the mapping from strings to the various graphics enums really belongs in the canvas code, not in the platform directory. -- 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] Issue Analysis:48290 [HTML Canvas globalCompositeOperation]
On Jun 1, 2011, at 10:14 AM, Mustafizur Rahaman wrote: Therefore, we submitted another patch where we are returning from the canvas code itself if the compositing mode is highlight. That’s not a good idea. We should keep looking into the real fix; that kind of hack is unneeded here. Similar issue (https://bugs.webkit.org/show_bug.cgi?id=48289 ) we have also seen for darker/CompositePlusDarker (not a valid compositing mode any more). Can we also remove this as well? That’s different. That mode is implemented, so we’d have to be sure it wasn’t used. We can’t remove it just because it’s not in the specification without investigating what content depends on it. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] New page for viewing test failures on build.webkit.org
Hi all- Before I go on vacation for 2.5 weeks, I wanted to let you know about a new page I've been working on on build.webkit.org. You can see it here: http://build.webkit.org/TestFailures/ The idea of the page is to provide a single place to go to find out what tests are failing on the bots and when they started failing. It also tries to make it easy to file bugs about the failures. It is pretty ugly, and has some glaring bugs (search Bugzilla for TestFailures). But I've found it useful already. I hope you will, too! Please file bugs and feature requests in the Tools / Tests component of Bugzilla, include the word TestFailures in the bug, and CC me. The code lives in Tools/BuildSlaveSupport/build.webkit.org-config/ public_html/TestFailures. Let me know what you think! -Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Cross-platform fonts for Layout Tests
I know that Ahem is safe to use across multiple platforms (the font metrics will be the same). Do we know if there are any other fonts for which this is true? I'd like to make the style-bot yell at people when they use pixel tests with non-safe fonts. Right now that list would only include ahem. -eric ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 11:31 PM, Maciej Stachowiak m...@apple.com wrote:
The following would be valid but would require us to cast away const all
over the place and would therefore in my opinion be a net negative:
const Node* previousSibling() const { return m_previous; }
And what's in our code now violates the intent of const, and so to me at
least is clearly a bug:
Node* previousSibling() const { return m_previous; }
I agree that these are both bad.
Well one big problem right now (just from scanning the core DOM classes) is
that we have a lot of clearly broken use of const. We could (a) leave it
as-is, (b) remove the incorrect use of const, or (c) deploy proper
const-correctness. it seems like you are against (b), but I cannot tell if
you advocate (a) or (c).
Misusing const is IMO worse than not using const. I would be in favor of
removing cases like the last one you cite above, as well as working to
remove const_casts, at least where they indicate problems.
It would be nice to make things more properly const-correct, and make
callers use const accessors when possible (e.g. use the const form of tree
iteration when walking a tree merely to read data and not write it), so I
don't think we should forbid good use of const, but it's also time-consuming
to retrofit.
Perhaps we could at least encourage const-correctness for newly-written
classes? By this I mean both adherence to the logical-constness rules you
stated earlier as well as not adding non-const accessors and members unless
needed -- otherwise it's easy to just err on the side of never using const
anywhere.
I also think we should not discourage people from using const on
variables. I've gotten review comments in the past that have asked me to
remove const qualifiers on locals, as well as sometimes on arguments and
members, and I think this is a mistake. We aren't const-correct elsewhere
in WebCore is not a good reason to forbid const.
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] New page for viewing test failures on build.webkit.org
Looks like a good start. Have you considered a test-centric view (instead of a bot-centric view)? That might make it easier to see what's going on globally across the project. Adam On Wed, Jun 1, 2011 at 12:22 PM, Adam Roben aro...@apple.com wrote: Hi all- Before I go on vacation for 2.5 weeks, I wanted to let you know about a new page I've been working on on build.webkit.org. You can see it here: http://build.webkit.org/TestFailures/ The idea of the page is to provide a single place to go to find out what tests are failing on the bots and when they started failing. It also tries to make it easy to file bugs about the failures. It is pretty ugly, and has some glaring bugs (search Bugzilla for TestFailures). But I've found it useful already. I hope you will, too! Please file bugs and feature requests in the Tools / Tests component of Bugzilla, include the word TestFailures in the bug, and CC me. The code lives in Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures. Let me know what you think! -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
Re: [webkit-dev] Pages from the wiki vanished from google search results
On Wed, Jun 1, 2011 at 5:27 PM, Robert Hogan li...@roberthogan.net wrote: Since a couple of days ago (when?), our trac/wiki is not appearing as relevant on google search results anymore. I noticed this yesteday too, but normal service appears to have been restored. From the two original examples I used to test, one is working as expected, the other not: not OK: http://www.google.com/search?hl=enq=webkit+team+%22Here+is+the+key+for+each%22 OK: http://www.google.com/search?hl=enq=Qt+Port+of+WebKit+%22List+of+all+QtWebKit+wiki+pages%22 Would be nice if someone with a google-webmaster account could investigate... Thanks, - Ademar -- Ademar de Souza Reis Jr. ademar.r...@openbossa.org Nokia Institute of Technology ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 11:31 PM, Maciej Stachowiak m...@apple.com wrote:
[...] What do you believe is the proper const-correct way to write
previousSibling() and related methods?
A priori, I think the correct way is this:
Node* previousSibling() { return m_previous; }
I could also be convinced that the following is technically more correct,
though in a way that is completely
useless for our code base at present:
const Node* previousSibling() const { return m_previous; }
Node* previousSibling() { return m_previous; }
What do you think is the right way to do it? One of these? Something else?
[...]
Well one big problem right now (just from scanning the core DOM classes) is
that we have a lot of clearly
broken use of const. We could (a) leave it as-is, (b) remove the incorrect
use of const, or (c) deploy proper
const-correctness. it seems like you are against (b), but I cannot tell if
you advocate (a) or (c).
I would *prefer* to deploy proper const correct accessors, so (c).
However, in the interests of pragmatism I think that it would be
reasonable to at least remove the improper uses of const (b).
From the tone of the initial e-mail it sounded like there was some
desire to get rid of const declarations across the board. I would be
opposed to this change. However, I concede that improper use of const
is worse than no const declaration, and would support your (b) case
(though I would prefer (c)!).
-Brent
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Different Versions of WebkitSupportLibrary.zip
Hi All, I am trying to build a fork of WebKit on windows that requires a version of WebKitSupportLibrary.zip different than the one currently available from http://developer.apple.com/opensource/internet/webkit_sptlib_agree.html In the script “WebKitTools/Scripts/update-webkit-support-libs” it’s looking for a version of WebKitSupportLibrary.zip with a checksum of a1341aadbcce1ef26dad2b2895457314 Does anyone know where I can download the correct version of WebKitSupportLibrary.zip?___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Do we have a style preference about const member functions?
On Tue, May 31, 2011 at 12:08 PM, Geoffrey Garen gga...@apple.com wrote: I agree that const should be used for logical constness. The rule should not be merely doesn't alter any data members of this object but rather does not alter observable state of this object or vend any type of pointer or reference by which observable state of this object could be altered. Precisely! I like this explanation too. I'm trying to think of a simple way to explain / test whether something falls into the category of logical constness, since it can be ambiguous. It occurred to me that a simple, though imperfect, test is just, Is this function called by an owner of a const pointer / reference? After all, a const member function is meaningless if nobody points to the class through a const pointer / reference. For classes like DOM and render tree nodes, which have no meaningful const-pointer-owning clients, the answer is always no. For other classes, the answer is yes, but only if someone has found a meaningful use for a const pointer or reference to the class. So, perhaps the real style question we should answer is when to use const pointers / references, since the answer to when to use const member functions will follow from it. What are some good examples of existing code that meaningfully uses a const pointer or reference? (Something other than, say, the obligatory const in a copy constructor.) I personally find that in a number of render tree functions declared to take a const RenderObject* that the const-ness is a useful hint that the function will not be manipulating the object in unexpected ways. Examples: http://codesearch.google.com/codesearch?hl=envert=chromiumlr=q=%22const+RenderObject%22+file%3AWebCoresbtn=Search (click Next at the bottom to see more results). My understanding is that these functions could not take a const RenderObject* parameter if RenderObject did not supply a set of const simple accessors and utility functions. We could convert RenderObject over to have no const member functions and convert all of these utility functions to take RenderObject* parameters instead, but I think that would lose some of the intent of the code. I'm curious if there was a specific patch or piece of code that lead you to send this email out - perhaps we make a better decision about whether to change our approach with more concrete examples of problems the current situation has caused or is causing. - James Geoff ___ 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
