Re: [webkit-dev] Do we have a style preference about const member functions?
On Jun 1, 2011, at 8:11 PM, James Robinson wrote: 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. Looks to me like these fulfill the requirement of do we ever use const pointers to these objects. So the next step is to fix up const member functions that inappropriately return non-const pointers to objects in the same tree (or that can otherwise get a back pointer). Make the functions non-const, then add const variants returning a const pointer if needed. I'm curious if there are any clients for const pointers to DOM nodes. Regards, Maciej ___ 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: 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 think const reference to String and const pointers to CSSStyleDeclaration/CSSMutableStyleDeclaration are good examples. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Cross-platform fonts for Layout Tests
Actually, even the same Ahem font will be rendered differently on different platform, depending on the font drawing library, the anti-aliasing algorithm, subpixel, tiny float-point calculation diff on different arch. On Thu, Jun 2, 2011 at 3:30 AM, Eric Seidel e...@webkit.org wrote: 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 ___ 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?
I think we should add a rule like a const member function should never return non-const pointer/reference. I have seen the problem in many places that you can get non-const reference on a const object. For example, InlineTextBox has InlineTextBox* prevTextBox() const; InlineTextBox* nextTextBox() const; Assume you have a function: void myFunction(const InlineTextBox* textBox) When the text box has a sibling, you can easily get a non-const pointer by calling textBox-nextTextBox()-prevTextBox(), which sounds ridiculous. 2011/5/31 Geoffrey Garen gga...@apple.com: Even in a class that is used in a tree, I still think simple member variable accessor methods (that do not return tree neighbors) should be const. OK. Why? Because it indicates to me and the compiler, that the method doesn't have side effects. A const member function can have side effects. It can modify any global state outside the object. It can also modify sub-objects inside the object, and return non-const references to sub-objects and related objects that might be used to produce side-effects at any time. It's exactly statements like this that make me think that const member functions are a bad idea -- people think they provide a guarantee, but they don't. 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
Re: [webkit-dev] Do we have a style preference about const member functions?
Similar thing: doc-frame()-document().
The solution should be defining both const and non-const members:
const Frame* frame() const { reutrn m_frame; }
Frame* frame() { return m_frame; }
2011/6/2 Yong Li yong.li.web...@gmail.com:
I think we should add a rule like a const member function should
never return non-const pointer/reference.
I have seen the problem in many places that you can get non-const
reference on a const object.
For example, InlineTextBox has
InlineTextBox* prevTextBox() const;
InlineTextBox* nextTextBox() const;
Assume you have a function: void myFunction(const InlineTextBox* textBox)
When the text box has a sibling, you can easily get a non-const
pointer by calling textBox-nextTextBox()-prevTextBox(), which sounds
ridiculous.
2011/5/31 Geoffrey Garen gga...@apple.com:
Even in a class that is used in a tree, I still think simple member
variable accessor methods (that do not return tree neighbors) should be
const.
OK. Why?
Because it indicates to me and the compiler, that the method doesn't have
side effects.
A const member function can have side effects. It can modify any global
state outside the object. It can also modify sub-objects inside the object,
and return non-const references to sub-objects and related objects that
might be used to produce side-effects at any time.
It's exactly statements like this that make me think that const member
functions are a bad idea -- people think they provide a guarantee, but they
don't.
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
Re: [webkit-dev] Cross-platform fonts for Layout Tests
I thought the whole point of Ahem was to avoid those problems. Adam On Thu, Jun 2, 2011 at 1:29 AM, Hao Zheng zheng...@chromium.org wrote: Actually, even the same Ahem font will be rendered differently on different platform, depending on the font drawing library, the anti-aliasing algorithm, subpixel, tiny float-point calculation diff on different arch. On Thu, Jun 2, 2011 at 3:30 AM, Eric Seidel e...@webkit.org wrote: 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 ___ 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] Cross-platform fonts for Layout Tests
Perhaps, but in practice, it's not enough. Here's an ahem pixel test that is slightly different on Mac and Chromium Linux: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/block/basic/010-expected.png http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/block/basic/010-expected.png Also, I think it would be hard to tell by examining the HTML if a test uses another font. For example, the line height of an empty block might depend on the default font that isn't specified (does pre/pre render the same height on all platforms?). On Thu, Jun 2, 2011 at 10:44 AM, Adam Barth aba...@webkit.org wrote: I thought the whole point of Ahem was to avoid those problems. Adam On Thu, Jun 2, 2011 at 1:29 AM, Hao Zheng zheng...@chromium.org wrote: Actually, even the same Ahem font will be rendered differently on different platform, depending on the font drawing library, the anti-aliasing algorithm, subpixel, tiny float-point calculation diff on different arch. On Thu, Jun 2, 2011 at 3:30 AM, Eric Seidel e...@webkit.org wrote: 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 ___ 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 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?
Responding to a few comments at once: 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. In https://bugs.webkit.org/show_bug.cgi?id=59604, Simon made a general comment that new methods should use const, but Alexey disagreed. Then I began this discussion, in the hopes of resolving the issue and writing a clear style guideline. I agree that concrete code is easier to discuss than abstract principles. Still, I'd like to write a general style guideline that coders and reviewers can use. On the specific question of accessors, it sounds like there's some consensus that accessors should be const, unless they return pointers or references, in which case accessors should be non-const, or should return const pointers or const references. In some cases, this may require two copies of the same accessor, but we'd rather not provide two copies of every accessor. Is that a fair summary? 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). I think const reference to String and const pointers to CSSStyleDeclaration/CSSMutableStyleDeclaration are good examples. All functions passed to enclosingNodeOfType in htmlediting.cpp are such clients: Thanks, this is a good set of examples. 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'm still not convinced that it's meaningful to talk about a const-correct class. The const that you put on a member function only has meaning when somebody uses a const pointer or reference to your class. More importantly, any design issues raised or bugs caught by const only get tested when somebody uses a const pointer or reference to your class. So, the key to const-correctness is deciding when to use const pointers and const references -- once you're using a const pointer or reference, the compiler will tell you which member functions need to be const. 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. I tend to agree. On a project-wide scale, it's an obvious contradiction, and a bit three-stooges-esque, for reviewers to ask for more const member function declarations and fewer const variable declarations. There is no const-correctness without const variable declarations. That said, I think const is most useful when used with a pointer or a reference, because it provides for a separation of concerns, and acts as a barrier between logically independent pieces of code that live in different source files but share data. Perhaps a data member also falls into this category, since it can be modified by any member function. In contrast, declaring a local int const will only protect you from contradicting your own intentions inside one block scope -- to me, that's not really worth the extra verbosity. However, in the interests of pragmatism I think that it would be reasonable to at least remove the improper uses of const (b). OK, it seems that there is also some consensus toward removing the const qualifier from member functions that return non-const pointers or references, or otherwise fail to meet the logical constness test. 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. I think I may have gone a little too far in venting my own confusion about the role of const member functions in C++ -- to stimulate discussion, I made a strawperson proposal not to use const member functions at all, but I don't think anyone actually supports that proposal, even though there is some support for removing incorrect or unhelpful uses of const member functions. Geoff ___ 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 Thu, Jun 2, 2011 at 1:02 PM, Geoffrey Garen gga...@apple.com wrote: 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'm still not convinced that it's meaningful to talk about a const-correct class. The const that you put on a member function only has meaning when somebody uses a const pointer or reference to your class. More importantly, any design issues raised or bugs caught by const only get tested when somebody uses a const pointer or reference to your class. So, the key to const-correctness is deciding when to use const pointers and const references -- once you're using a const pointer or reference, the compiler will tell you which member functions need to be const. Hmm. Relying on compilers to check constness is inherently scary because they check physical constness, which is neither a superset nor subset of logical constness, which is what we want to be checking. At least in my head, my design process is generally the class should expose as few APIs as possible, and they should be const wherever possible (without violating logical constness). Obviously in some sense classes aren't built in a vacuum and their external users determine the APIs they expose, but I don't like the constness is determined purely by external need route because it leads to problems like the first user of this class wanted to mutate everything, so I made no members const, and now I can't trivially add a non-mutating const usage because it requires plumbing constness through everywhere. Maybe what I'm trying to say is that it's generally feasible to design with make usage patterns const where possible in mind, but it's harder to retrofit. Or maybe I'm saying it's easier to make things non-const later than to make them const. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] LocalStorage spec and structured cloning
Does anyone have an opinion on this Web Storage spec bug? The input of the WebKit community is desired. And probably Safari and Chrome folks in particular, if opinions differ. http://www.w3.org/Bugs/Public/show_bug.cgi?id=12111 http://dev.w3.org/html5/webstorage/#dom-storage-getitem says that The getItem(key) method must return a structured clone of the current value associated with the given key. but all browsers I've tested return a string representation of the value instead. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] window.internals is teh hot. Help make it work on all platforms
As of http://trac.webkit.org/changeset/87948, we have a new, less painful way to write WebCore tests: the window.internals object. Similar to window.layoutTestController, this object is only exposed when DRT runs. Unlike window.layoutTestController, you don't have to wade in every port's implementation of DumpRenderTree and plumb the calls through each port's layer to implement your WebCore-related layout test methods. As an example, see how simple it is to expose all shadow DOM-related methods with window.internals: https://bugs.webkit.org/show_bug.cgi?id=61671. This is possible, because the implementation of the window.internal object actually lives in WebCore (http://trac.webkit.org/browser/trunk/Source/WebCore/testing/), but is built as a separate library, which is linked into DumpRenderTree. There is a tiny bit of port-specific code that then facilitates construction and injection of the object instance, allowing the actual testing code to stay port-independent. Obviously, this is not a replacement for window.layoutTestController. Here's a set of simple rules you should follow when deciding where to add your testing hooks: 1) Does the code you are testing reside entirely in WebCore? If yes, use window.internals. 2) Otherwise, use window.layoutTestController. End-to-end tests are still a huge part of our testing infrastructure. Finally, A CALL TO ACTION. Currently, the window.internals object only work on platform/mac and platform/chromium. We need heroes to help implement it for other ports. If you are a hero, please fix one of these bugs: https://bugs.webkit.org/show_bug.cgi?id=61071 https://bugs.webkit.org/show_bug.cgi?id=61074 https://bugs.webkit.org/show_bug.cgi?id=61076 Thank you, heroes! And thank you Hajime Morrita, Darin Adler, Alexey Proskuryakov, and Mark Rowe for ideas and direction. :DG ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] LocalStorage spec and structured cloning
I'm concerned that implementing this will only encourage more use of localStorage. The API is very poor because it requires synchronous IO and synchronization between browser contexts, which may live on different threads. (Note: Chrome does not implement the required synchronization.) If we could fix localStorage to be asynchronous and transactional :-) then it'd be cool. Of course, one answer is that people should just use IndexedDB. FWIW, Jorlow (when he was still working on chrome) expressed similar sentiments. On Jun 2, 2011 4:13 PM, Maciej Stachowiak m...@apple.com wrote: Does anyone have an opinion on this Web Storage spec bug? The input of the WebKit community is desired. And probably Safari and Chrome folks in particular, if opinions differ. http://www.w3.org/Bugs/Public/show_bug.cgi?id=12111 http://dev.w3.org/html5/webstorage/#dom-storage-getitem says that The getItem(key) method must return a structured clone of the current value associated with the given key. but all browsers I've tested return a string representation of the value instead. Regards, Maciej ___ 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] LocalStorage spec and structured cloning
I'd advise commenting right in the w3c bugzilla, but if you don't feel like making an extra account I can copy in the below remarks for you. - Maciej On Jun 2, 2011, at 8:54 PM, Darin Fisher wrote: I'm concerned that implementing this will only encourage more use of localStorage. The API is very poor because it requires synchronous IO and synchronization between browser contexts, which may live on different threads. (Note: Chrome does not implement the required synchronization.) If we could fix localStorage to be asynchronous and transactional :-) then it'd be cool. Of course, one answer is that people should just use IndexedDB. FWIW, Jorlow (when he was still working on chrome) expressed similar sentiments. On Jun 2, 2011 4:13 PM, Maciej Stachowiak m...@apple.com wrote: Does anyone have an opinion on this Web Storage spec bug? The input of the WebKit community is desired. And probably Safari and Chrome folks in particular, if opinions differ. http://www.w3.org/Bugs/Public/show_bug.cgi?id=12111 http://dev.w3.org/html5/webstorage/#dom-storage-getitem says that The getItem(key) method must return a structured clone of the current value associated with the given key. but all browsers I've tested return a string representation of the value instead. Regards, Maciej ___ 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] LocalStorage spec and structured cloning
I think that these shortcomings are also strengths. A synchronous localStorage is a drop-in replacement for cookies, which is a good thing to offer, and to encourage. Practically, if it's only cookies vs. IndexedDB (or SQL Database), I'd realistically choose the former for most code, and I'm sure that many Web developers would do the same. It doesn't matter all that much if developers encode to JSON themselves or the engine does that, but adding a little syntactic sugar seems beneficial. - WBR, Alexey Proskuryakov 02.06.2011, в 20:54, Darin Fisher написал(а): I'm concerned that implementing this will only encourage more use of localStorage. The API is very poor because it requires synchronous IO and synchronization between browser contexts, which may live on different threads. (Note: Chrome does not implement the required synchronization.) If we could fix localStorage to be asynchronous and transactional :-) then it'd be cool. Of course, one answer is that people should just use IndexedDB. FWIW, Jorlow (when he was still working on chrome) expressed similar sentiments. On Jun 2, 2011 4:13 PM, Maciej Stachowiak m...@apple.com wrote: Does anyone have an opinion on this Web Storage spec bug? The input of the WebKit community is desired. And probably Safari and Chrome folks in particular, if opinions differ. http://www.w3.org/Bugs/Public/show_bug.cgi?id=12111 http://dev.w3.org/html5/webstorage/#dom-storage-getitem says that The getItem(key) method must return a structured clone of the current value associated with the given key. but all browsers I've tested return a string representation of the value instead. Regards, Maciej ___ 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 mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
