Re: [webkit-dev] Compiling WebKit up to 25% faster in Windows?
On Tue, Mar 26, 2013 at 5:51 PM, Benjamin Poulain benja...@webkit.orgwrote: Hackabilty is a project goal. Compile time is not. Well, in fairness, I think anyone contributing seriously to a codebase will get more hacking done if they're spending significantly less time recompiling :). I happen to be someone who finds full paths more readable and more instructive as to the true dependencies of a file, but I think Benjamin's concern about not wanting it to be too hard to reorganize files is reasonable. Developing a moderately-robust script to help fix up #includes when moving a file would probably help. I also share the viewpoint that these initial results are interesting but not yet broad enough to make final decisions. It seems like it's worth the time to try and find out whether the gains generalize across WebKit and on other platforms/compilers. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please don't leave entries for rebaseline in TestExpectation files
On Wed, Mar 20, 2013 at 11:46 PM, Robert Hogan li...@roberthogan.netwrote: On Wednesday, 20 March 2013, Ryosuke Niwa wrote: Please don't add lines to TestExpectations saying that they just need rebaselines and then leave. OK. That means I will have to pull the new results from the bots, which is fine - but in the case of the Mac port (and any other bot that does not run pixel tests) the result will be that trunk will get fresh text results but retain stale png results. I suspect by and then leave Ryosuke meant and never come back. It seems reasonable to me to check in and then wait a sufficient amount of time for the bots to cycle fully before using garden-o-matic to pull the correct baselines. This would mean we might have people leaving a [pkasting] Will rebaseline this test before Mar. 22, 2013 line on some expectations for a day or two, just not forever. We've batted back and forth on this list for at least a year on the correct approach for landing and rebaselining. My approach is to land results for the platform that I build, suppress tests that require rebaselining on other platforms, and open a bug so sheriffs can add/rebaseline results as appropriate. It's certainly nicer than not landing any expectations :). But as the current Chromium WebKit sheriff, I just spent a few hours rebaselining a lot of these sorts of things in the Chromium expectations, some of which had been around for months. It's easy for these sorts of needs rebaseline bugs to get lost in the shuffle, and in a few cases, I couldn't determine if needs rebaseline was still correct due to further changes that had happened since. For these reasons, the original change author is in the best position to ensure the right rebaselining happens quickly, although I do realize that this is a nontrivial burden to place on change authors. I don't know if that means we should say do this if you can, but OK if not, or what. My impression from recent discussion on this topic was that this was the way that worked best for everybody.I used to pull results from the bots where possible but creating inconsistency between png/text results is not good. As long as the relevant bots have all cycled past the change in question, your checkout contains all the relevant LayoutTest subdirectories, and you've updated to ToT, I believe garden-o-matic can correctly rebaseline any of the ports it supports, regardless of whether you can build/run those ports locally. Inconsistencies I've seen have been a result of non-updated checkouts or non-cycled bots. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Mon, Mar 11, 2013 at 2:59 PM, Ryosuke Niwa rn...@webkit.org wrote: Is it expected that overtype works on Windows or on Linux? e.g. If Edit and RichEdit window classes both support this feature natively on Windows (which I bet they do), then the answer is yes. The non-rich edit control on Windows (i.e. Notepad) does not support overtype mode. The rich edit control (Wordpad) does support it. MS Word also uses a rich edit control, and goes further by displaying an explicit normal vs. overtype indicator in its UI. The Chrome omnibox uses a native rich edit control, but in early testing overtype mode always ended up being confusing and annoying, so we disabled it, and we've never had a request to re-enable. I'm not necessarily opposed to plumbing support for overtype mode, but I suspect it may not make sense for all text input controls, e.g. single-line controls; and even if we support it, we may want some mechanism to show the user what mode they're in. Without a visual indicator, it can be surprising and frustrating (which is how it feels to me in Wordpad). PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Mon, Mar 11, 2013 at 4:21 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Mar 11, 2013 at 3:56 PM, Peter Kasting pkast...@google.comwrote: On Mon, Mar 11, 2013 at 2:59 PM, Ryosuke Niwa rn...@webkit.org wrote: Is it expected that overtype works on Windows or on Linux? e.g. If Edit and RichEdit window classes both support this feature natively on Windows (which I bet they do), then the answer is yes. The non-rich edit control on Windows (i.e. Notepad) does not support overtype mode. The rich edit control (Wordpad) does support it. MS Word also uses a rich edit control, and goes further by displaying an explicit normal vs. overtype indicator in its UI. I see. We already have a notion of editable vs. richly editable so we can enable this feature only inside a richly editable area. I'm not necessarily opposed to plumbing support for overtype mode, but I suspect it may not make sense for all text input controls, e.g. single-line controls; and even if we support it, we may want some mechanism to show the user what mode they're in. We should match whatever the platform norm is. Well, that's the thing. On Windows there isn't really a platform norm. Even a distinction like editable versus richly editable is not really a user-level concept in Windows, it's more of an implementation distinction, and there definitely is no obvious pattern for which applications or text fields will support overtype mode, or whether there's some sort of visible indicator of it. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Mon, Mar 11, 2013 at 5:11 PM, Ryosuke Niwa rn...@webkit.org wrote: Of course, each application can implement overtype in Edit window class and manually disable it in RichEdit window class but I don't think that's an interesting fact since that's a customization. An application doesn't even have to use either window class to implement a text field; e.g. Microsoft Word. (Word actually uses a rich edit control just like Wordpad does, which is why Office ships a newer version of that control's DLL.) But that doesn't mean there is no platform norm. Just like un-customized NSTextView establishes what's norm on Mac, un-customized Edit and RichText establish what's norm on Windows to a certain extent. I think that might make sense from a programmer's perspective, though even there I think you're on thin ice on Windows -- there are so many different controls and frameworks for doing UI that the UX on Windows is much more fragmented in general than on Mac (long one of the complaints of those who prefer Mac OS). I know what the MFC CEdit and CRichEditCtrl do, but I don't anything about, say, WPF UIs. I question whether it makes sense at all from a user's perspective. Users don't know or care what classes are used to implement their applications, they care what the applications do. And from that perspective Windows is all over the place. Wordpad, Word, and Visual Studio all support overtype mode, but all three do it differently (Visual Studio is IMO the best -- it actually changes the cursor from bar to block in overtype mode, which I'd love to see any other app supporting this to adopt, and certainly would want if we turned this on in Chrome for Windows). Notepad, which to almost any user who doesn't open files with LF line endings is the same thing as Wordpad, doesn't, and neither does Firefox or Chrome (today). Third-party editors are all over the place. I think we should decide this based on what is most helpful and least confusing to users. Normally a platform convention is a way of establishing what users will expect, which is why it has any value at all. Here I'm not convinced there's a user expectation of overtype mode in general, and I definitely worry that it has low utility and high annoyance, which make me greatly hesitate. And implementing it for rich text but not plain text on the web seems like a recipe for confusion too -- do normal users understand the distinction and would expect overtype mode in one but not the other? Seems more likely that some people would be frustrated that overtype mysteriously doesn't work sometimes, while others would be confused why every once in a while their text gets eaten, but not in a way they can reproduce consistently. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Mon, Mar 11, 2013 at 5:54 PM, Ryosuke Niwa rn...@webkit.org wrote: Having said that, I object to implementing a behavior doesn't match RichEdit or Edit window classes on Windows. We should match either native edit window class. Are you referring to my comments about the cursor? Do you object, then, to other behaviors the native controls don't support, e.g. triple-click to select all (not part of at least some versions of CRichEditCtrl, which is why I hand-implemented it in the Chrome omnibox)? In general, if we have a superior way of doing something, are we to be forced to avoid implementing it because Microsoft didn't get around to adding it to CRichEditCtrl? Let's argue about things like block-cursor-in-overtype-mode from a is it good for users perspective, not from a what does class XYZ that ships with Windows do perspective. That's the wrong priority. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Mon, Mar 11, 2013 at 7:01 PM, Ryosuke Niwa rn...@webkit.org wrote: Yes, we should disable triple-click-to-select-all on Windows You realize that Wordpad, Word, and Internet Explorer all support this, though, right? My point was not just to raise a behavior that seems like a clear win, it was to reiterate that there's little connection between what a control does and what users will expect to see. Triple-click is widely used on Windows and taking it away would be clearly wrong from a user expectation perspective. Granted, there are certain editing features that are hard to replicate native behavior (e.g. caret movements in bidirectional text) but those should still be considered as bugs. There are also certain cases where the native control is just clearly buggy. For example,CRichEditCtrl draws bad selection highlights and adds an extra newline to the displayed string when used in single line mode, apparently because Microsoft never actually used it in single line mode and thus didn't test. There are other cases (which I forget) where certain actions just cause bizarre behavior that makes no sense and is clearly wrong. It makes no sense to me to replicate these sorts of things. I remember I was stunned by how selection was painted in Google Chrome on Windows when it was initially released because it violated the platform convention I was used to on Windows. There were quite few other editing-related features that struck me as unconventional such as the caret appearing before the space following a word when moving caret forward at word boundaries. All those tiny differences added up to the point where I decided not to use Google Chrome as my primary Web browser on Windows. Things like the caret movement were clearly wrong. Everything everywhere on Windows does caret movement a certain way and you're far from the only person who had problems with those sorts of things (I complained to Ojan about something like a hundred different such bugs). Even today there are still some lurking bugs when trying to expand the selection range on windows using shift+up or shift+down, and some other things that are obviously wrong. It's also worth noting that some behaviors cause usage issues and others don't. Getting the caret movement wrong, for example, causes concretely wrong output when someone attempts to rapidly edit text using the caret movement shortcuts they're familiar with. In the case of showing a blinking block cursor for overtype mode, not only is Windows inconsistent about how it handles things, but this purely visual change can't cause users to wind up with the wrong text. Indeed, the entire point is to point out that overtype mode is enabled -- which is not what the user intended 99% of the time anyway -- and help PREVENT incorrect output more quickly. So I'm extremely skeptical when you say we can up with a superior way of doing something on Windows based on user expectations because user expectations are often shaped by other applications they use. Thank you for reiterating my previous point: user expectations are shaped by how things actually behave, not what the underlying controls implement at a code level. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Mon, Mar 11, 2013 at 8:54 PM, Shezan Baig shezbaig...@gmail.com wrote: I feel like I should give some background to this discussion. Thanks for this context. It's helpful. So I guess the question boils down to something like: if we have changes that are generally useful, but not used in the major WebKit applications (i.e. Chrome, Safari, Opera), does it make sense to upstream it to WebKit for the benefit of the general community? Disclaimer: I'm not the gatekeeper of the WebKit codebase. Someone like Maciej might be a better judge of this. If none of the public WebKit ports intended to use the feature, I would say it doesn't belong upstream. Similarly, if some of the ports thought they might want to implement UI for this someday, but weren't presently doing so, I wouldn't upstream the core capability until at least one port actually begins implementing such UI. So perhaps the question is not so much would overtype mode be potentially interesting as are any public ports interested in exposing overtype mode to their users in the near term, and would dedicate someone to work on it. If yes, then upstreaming low-level hooks for this seems reasonable. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Merging the iOS port to svn.webkit.org (Re: WebCore and interlocking threads)
On Sat, Feb 9, 2013 at 11:52 AM, Maciej Stachowiak m...@apple.com wrote: There are certainly pros and cons to merging. It would be great get input from the broader WebKit community on the tradeoff of merging sooner vs avoidance of weird legacy code in the main tree. In the meantime, we'll stick to merging things that are not overly controversial as much as we can. For what my opinion is worth (probably near zero for a lot of you), I would like to see you guys merge sooner rather than later, even if it leads to awkwardness that needs cleanup. Over the years there has been a nonzero amount of friction due to the iOS port not being upstreamed, and I think it would be beneficial to WebKit as a whole to fix that sooner rather than later. And it seems more likely to me that upstream first, then decide how to re-architect as needed is going to result in high-quality discussions and designs, as opposed to figure out in private how to re-architect before upstreaming, which runs the risk of just never bothering to upstream at all. There is real compromise, and perhaps humility, needed from all sides to make such a task successful. I am reminded of Eric's recent email where he pleaded for more of an explicit effort to civility towards each other. Perhaps this is an opportunity for us to practice that effort. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
On Thu, Jan 3, 2013 at 11:36 AM, Shawn Singh shawnsi...@chromium.orgwrote: Cons of making a separate vector class: - offsets are sometimes treated as relative point locations, and other times treated as vectors that can be added to points. Deciding when to use which one could become just as confusing as using Point vs Size is right now. Yeah, this is a real danger. It's sort of mitigated if you have no way to add/subtract two points, only a point and a vector, because that kind of forces you to always use your vector class for offsets, otherwise you can't do much with them. However, you do still end up needing things like PointAtOffsetFromOrigin(const vector) that basically just convert a vector directly to a point, so there is still the possibility for confusion. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
On Wed, Jan 2, 2013 at 11:21 PM, Steve Block stevebl...@chromium.orgwrote: - Would people welcome changes to encourage that policy? FWIW, in Chromium (downstream, non-WebKit) we ended up adding a vector (as in math, not the STL) class recently to address the sort of offset/delta between two points use you're describing, because neither points nor sizes made for a good fit all the time. The folks working on Chromium's compositor might have more feedback, but personally I thought that was a good move, and it turned out to not be too hard to convert almost everything at once, simply by removing all the functions implementing mathematical operations between type pairs we no longer wanted to support and seeing what broke. Of course, such a drastic move in WebKit would need to have significant buy-in ahead of time, and my suspicion is that it wouldn't get it. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 1:11 PM, Emil A Eklund e...@chromium.org wrote: That said, if your strong reason turned out to be incorrect, you should recommit the patch, no? That seems like a bad idea, someone that understands the patch should recommit it. Ideally the original author. I don't understand your logic. A patch landed, the sheriff thinks maybe it was bad and rolls it out, then it turns out it was a red herring. Why is it not now the sheriff's responsibility to re-land? After all, the patch was landed originally by people who understood it and hasn't been seen to cause any problems. On the occasions when I've had to roll-out to diagnose an issue, I've always re-landed patches that it turns out weren't broken. Not doing this seems not only extremely rude but actively dangerous to the health of the tree, since other changes may now be landed or near-landing that depend on this change. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 1:19 PM, Emil A Eklund e...@chromium.org wrote: I don't understand your logic. A patch landed, the sheriff thinks maybe it was bad and rolls it out, then it turns out it was a red herring. Why is it not now the sheriff's responsibility to re-land? After all, the patch was landed originally by people who understood it and hasn't been seen to cause any problems. There might very well have been other changes that conflicts with it. If it applies cleanly then I agree with you that whoever rolled it out should reland it. If there are conflicts or if it requires merging in any way though I'd argue that the original author needs to get involved. There are certainly cases where the original author needs to be involved, but I'd be happy just saying this is a judgment call. Usually rollouts happen not long after a patch lands, and roll-ins happen not long after that. In those cases, most merge failures are trivial and mechanical and can easily be handled by a conscientious sheriff who reads the relevant changes involved in the conflicts. Sometimes, of course, that's not true. But sheriffs should be biased towards try to leave working patches in the tree. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 2:14 PM, Oliver Hunt oli...@apple.com wrote: I don't understand why anyone is _speculatively_ rolling out patches. You should only be rolling it out if you _know_ the patch is bad. Sometimes something bad happens to the tree, the sheriff doesn't know which patch is responsible, and the change authors are not present to ask for help. In a case like this the sheriff has to either do speculative rollouts or leave the tree broken. Ideally, of course, change authors are around when something like this happens. But maybe the bustage doesn't happen until much later, due to some subtle/latent issue, or maybe the change author is in fact irresponsible. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 2:20 PM, Oliver Hunt oli...@apple.com wrote: Or the sheriff could actually see if rolling out a patch locally fixes the problem. I'm not sure why they're considering not testing to be a valid behaviour for someone who is ostensibly meant to be keeping things going in the face of people who aren't testing. If the sheriff is capable of testing locally, that's an option. It's often impossible, however, for the sheriff to test locally, e.g. if the bustage is in a port he can't build. Even when possible, it may take a prohibitively long time to sync, build, and test, during which time the tree is broken for everyone. Cycling the main waterfall itself may inconvenience the rest of the developer community less. As usual, it's a judgment call. Again, I've spent many days as WebKit sheriff, and I've only done speculative rollouts a couple of times, so I don't see this as a constant, major problem. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] moving focus when clicking on scrollbars
It seems worth noting over here that on the whatwg discussion for this, native really means Mac and Ubuntu. Notably it's not clear whether this matches Windows, which is 90+% of the userbase for Chrome. I am a little nervous making blanket statements like this is native behavior when we're not sure whether it is for such large user groups. I'm not sure how to test this on Windows, though. Gecko's behavior makes me a little less worried than the never move focus behavior in the absence of data to answer the above question. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] moving focus when clicking on scrollbars
On Wed, Oct 31, 2012 at 1:32 PM, Ojan Vafai o...@chromium.org wrote: Every native platform that has scrollbars does *not* move focus when you click on them. Every browser engine, except Gecko, moves focus when you click on scrollbars *unless* you're clicking on the viewport scrollbar (e.g. clicking on the scrollbar of an scrollable div that fills the viewport will move focus). Gecko does not move focus when you click on any scrollbar unless you are clicking on the scrollbar of a form control (e.g. textarea) scrollbar. I'd like to change our behavior to either match Gecko or go fully native and never move focus when clicking on scrollbars. The latter sounds better to me, but either solution would satisfy me. Is there rationale for Gecko's behavior? It sounds a bit strange. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] moving focus when clicking on scrollbars
On Wed, Oct 31, 2012 at 2:40 PM, Ojan Vafai o...@chromium.org wrote: On Wed, Oct 31, 2012 at 2:29 PM, Peter Kasting pkast...@chromium.orgwrote: Is there rationale for Gecko's behavior? It sounds a bit strange. Not that I know of. I haven't talked to anyone at Gecko about it though. Might be nice to try and find someone appropriate there to ping. Surprised the topic didn't come up as path of the whatwg discussions you mentioned (since it's usually good to understand why the world is the way it is as a starting point). PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] On returning mutable pointers from const methods
On Sun, Oct 28, 2012 at 11:16 PM, Maciej Stachowiak m...@apple.com wrote: On Oct 28, 2012, at 10:09 PM, Peter Kasting pkast...@chromium.org wrote: On Sun, Oct 28, 2012 at 6:12 AM, Maciej Stachowiak m...@apple.com wrote: I am not sure a blanket rule is correct. If the Foo* is logically related to the object with the foo() method and effectively would give access to mutable internal state, then what you say is definitely correct. But if the const object is a mere container that happens to hold pointers, there's no particular reason it should turn them into const pointers. For example, it is fine for const methods of HashSet or Vector to return non-const pointers if that happens to be the template parameter type. In such cases, constness of the container should only prevent you from mutating the container itself, not from mutating anything held in the container, or else const containers of non-const pointers (or non-const types in general) would be useless. IMO const containers that vend non-const pointers _are_ nearly useless. I consider logical constness to include not only this statement has no observable side effect but also this statement does not allow me to issue a subsequent statement with observable side effects. Surely that's not quite correct as a definition of logical constness. You can always pass a const reference to another object's setter to cause an observable side effect. The scope of side effects under consideration has to be limited to the object itself plus anything else that could be considered part of its state. In brief, a const method on object O should neither mutate O nor expose the possibility of mutating the state of O, but it has no responsibility for its involvement in mutation of objets Sorry, I left out words. Consider the phrase on the state of the object to be appended to both those quoted phrases I originally said. I still think that it's extremely difficult to avoid exposing the possibility of mutating the state of O in most cases. The mechanism may be obscure, but it is frequently present -- frequently enough to make me advocate for hard-and-fast rules. Consider the following use case: - I have collected a an ordered list of pointers to objects of type T in a Vector. - I'd like to call a function that will operate on this list - it's allowed to do anything it wants to the Ts, including mutating them, but it can't alter the order or contents of the Vector. (For example, I may want to pass the same list to another function). Currently one would express this by passing a const VectorT*. I don't see a good approach to this that strictly follows the suggested rule. You'd have to either copy the Vector to a temporary just for the call, or abandon const-correctness. I think by abandon const-correctness you mean pass a VectorT**, which is indeed the route I'd go. I don't consider that an abandonment of const-correctness, in that you are indeed not violating logical constness. But yes, you lose the ability to convey the idea that this function, in and of itself, doesn't modify the number or order of elements in the vector. On the other hand, you don't put yourself in a position where a caller of the function could then immediately use his returned T* to mutate the vector -- which is often a real possibility in real-world systems. There aren't any magic bullets here. Given that the compiler can very rarely use const to optimize anything anyway, const is effectively whatever we want it to be. I prefer hard guarantees that occur less frequently to less-ironclad guarantees that are more common. Reasonable people can disagree. I think your position -- that the hard guarantee usually makes sense for non-simple-containers but you'd prefer to allow a less-strict usage for simple containers -- is one that has some appeal even if it's not what I'd personally choose (and is more complex to explain/enforce). I certainly think we'd be in a better world if the codebase followed this policy, compared to today. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] On returning mutable pointers from const methods
On Sun, Oct 28, 2012 at 6:12 AM, Maciej Stachowiak m...@apple.com wrote: I am not sure a blanket rule is correct. If the Foo* is logically related to the object with the foo() method and effectively would give access to mutable internal state, then what you say is definitely correct. But if the const object is a mere container that happens to hold pointers, there's no particular reason it should turn them into const pointers. For example, it is fine for const methods of HashSet or Vector to return non-const pointers if that happens to be the template parameter type. In such cases, constness of the container should only prevent you from mutating the container itself, not from mutating anything held in the container, or else const containers of non-const pointers (or non-const types in general) would be useless. IMO const containers that vend non-const pointers _are_ nearly useless. I consider logical constness to include not only this statement has no observable side effect but also this statement does not allow me to issue a subsequent statement with observable side effects. A const Vector that allows to to obtain a non-const element pointer, which you subsequently mutate, means that you can mutate the vector. (Consider for example an element destructor that removes the element from the vector.) If you allow const methods to return non-const pointers, it's almost always possible to construct some chain of events that leads to mutation of the const object or of state it can observe. It is far easier to simply make simple rules like const methods shall not vend non-const pointers than try to allow it but guarantee that all the code everyone writes is safe. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] On returning mutable pointers from const methods
On Fri, Oct 26, 2012 at 8:27 AM, Rik Cabanier caban...@gmail.com wrote: It is valid for a const method to return you a new object ie a const factory object. In that case, const-ness would not be desired. Not really. The point of this thread is that such functions may not modify an object's state themselves, but they vend access that can be used by the caller to modify it. Consider for example: Child* Parent::getNewChild() const; Assuming the Parent doesn't have a list of its children (questionable), we can implement this without mutable pointers. But then a caller can do: Child* child = parent-getNewChild(); child-parent-mutate(); If you generalize this you find there are very, very few cases where a const object can vend a non-const pointer that cannot possibly be used to change the state of the world the const object sees. Which is why the rule of thumb suggested in this thread is safer and easier than trying to reason about individual cases. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] A proposal for handling failing layout tests and TestExpectations
On Wed, Aug 15, 2012 at 5:00 PM, Filip Pizlo fpi...@apple.com wrote: I believe that the cognitive load is greater than any benefit from catching bugs incidentally by continuing to run a (1-fail) or (3) test, and continuing to evaluate whether or not the expectation matches some notions of desired behavior. As someone who has spent a lot of time maintaining Chromium's expectations, this seems clearly false, if your proposed alternative is to stop running the test. This is because a very common course of events is for a test to begin failing, and then later on return to passing. We (Chromium) see this all the time with e.g. Skia changes, where for example the Skia folks will rewrite gradient handling to more perfectly match some spec and as a result dozens or hundreds of tests, many not explicitly intended to be about gradient handling, will change and possibly begin passing. By contrast, if we aren't running a test, we don't know when the test begins passing again (except by trying to run it). The resulting effect is that skipped tests tend to remain skipped. Tests that remain skipped are no better than no tests. And even if such tests are periodically retested, once a test's output changes, there is a large window of time where the test wasn't running, making it difficult to pinpoint exactly what caused the change and whether the resulting effect is intentional and beneficial. If we ARE running a test, then when the results change, knowing whether the existing result was thought to be correct or not is a critical part of a sheriff's job in deciding what to do about the change. This is one reason why Chromium has never gone down the path of simply checking in failure expectations, and something that Dirk's proposal explicitly tries to address while still allowing ports that (IMO mistakenly) don't care to continue to not care. We already have some good tooling (e.g. garden-o-matic) that could be extended to show and update the small amount of additional info Dirk is proposing. I am very skeptical of abstract claims that this proposal inflates complexity and decreases productivity in the absence of actually testing a real workflow using the tools that we sheriffs really use to maintain tree greenness. I would like to see this proposal tested to get concrete feedback instead of arguments on principle. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] A proposal for handling failing layout tests and TestExpectations
On Wed, Aug 15, 2012 at 5:45 PM, Filip Pizlo fpi...@apple.com wrote: The typical approach used in situations that you describe is to rebase, not skip. Which is precisely what Dirk is proposing. Literally the only difference here is that the rebased test expectation would contain an optional notation about whether we believe the new baseline to be correct or incorrect. Ports that don't care, or tests where we don't know, will be totally unaffected. I am not sure why this bothers you so much. You talk about making the infrastructure more complicated, but it doesn't seem like there is much additional complication involved, and what minor complication is involved is being volunteered to be handled by Dirk and other folks. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] A proposal for handling failing layout tests and TestExpectations
On Wed, Aug 15, 2012 at 6:02 PM, Filip Pizlo fpi...@apple.com wrote: 2) Possibility of the sheriff getting it wrong. (2) concerns me most. We're talking about using filenames to serve as a kind of unchecked comment. We already know that comments are usually bad because there is no protection against them going stale. I don't see how this is parallel to stale comments. Tests get continually compared to the given output and we see immediately when something changes. It is certainly possible for whoever assigns the filenames to get things wrong. There are basically two mitigations of this. One is allowing the existing expected.xxx file extensions to remain, and encouraging people to leave them as-is when they're not sure whether the existing result is correct. The other is for sheriffs to use this signal as just that -- a signal -- just as today we use the expected.xxx files as a signal of what the correct output might be. The difference is that this can generally be considered a stronger signal. Historically, there's been no real attempt to guarantee that an expected result is anything other than the test's current behavior. I'm sure some would love to get rid of Skipped files just as much as I would love to get rid of TestExpectations files. Both are valid things to love, and imply that there must surely exist a middle ground: a way of doing things that is strictly better than the sum of the two. That's exactly what we're trying to do. The value of this change is that hopefully it would dramatically reduce the amount of content in these, especially in TestExpectations files. If you want to kill these so much, then this is a change you should at least let us test! In particular, to further clarify my position, if someone were to argue that Dirk's proposal would be a wholesale replacement for TestExpectations, then I would be more likely to be on board, since I very much like the idea of reducing the number of ways of doing things. Maybe that's a good way to reach compromise. It's hard to know if we could completely eliminate them without testing this, but yes, one goal here is to greatly reduce the need for TestExpectations lines. A related goal is to make the patterns and mechanisms used by all ports more similar. As someone who has noted his frustration with both different ways of doing things and philosophical directions chosen by one port, you would hopefully be well-served by this direction. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Status of multithreaded image decoding
On Sun, Aug 12, 2012 at 1:24 PM, Maciej Stachowiak m...@apple.com wrote: Why not start asynchronous decoding immediately as the image is loading, and synchronize at paint time? What is the benefit of waiting until layout time to start decoding the image data? Uninformed guess (since I haven't touched the decoders in a while), but currently we don't decode unless the image is actually painted, which helps a ton on pages that are an enormous long string of images (common cases: Boston Big Picture blog, various porn sites), since most of the images can be decoded after initial layout, or not at all (if the user never scrolls down enough). If we started decoding as images loaded I'd imagine we'd do (possibly a lot of) extra work compared to today. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] trac.webkit.org timeline broken
It looks like the timeline has once again stopped updating. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] trac.webkit.org timeline broken
The Trac timeline doesn't seem to have updated since last night. It's missing about 15 commits right now. Trying to link to one of these -- e.g. http://trac.webkit.org/changeset/123971 -- doesn't work. Not sure to whom to direct this problem. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: rebaseline tooling
On Thu, Jul 12, 2012 at 3:17 PM, Ojan Vafai o...@chromium.org wrote: https://trac.webkit.org/wiki/Rebaseline I've recently consolidated the various rebaseline commands and made the tool work for non-Chromium ports. I especially like webkit-patch rebaseline path/to/test/i/just/broke.html, which lets you easily rebaseline the test for all ports once the bots have cycled. Thanks Ojan! On a random note since you mentioned bots cycling. Is there any way for this tool/garden-o-matic to detect whether the bots it's pulling results from have cycled since a particular change? I've been bitten (and I know others have too) by trying to pull new baselines too early and then having to do it again, and having the tool tell me, say, the most recent revision that it has complete results for would be awesome. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] PSA: rebaseline tooling
On Thu, Jul 12, 2012 at 4:16 PM, Dirk Pranke dpra...@chromium.org wrote: At the top of the garden-o-matic page there is a line like Latest revision processed by every bot: 122499 (trunk is at 122524). I think that does what you want? Ah, I hadn't noticed that. That sounds promising! PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] TestExpectations syntax changes, last call (for a while, at least) ...
On Thu, Jun 14, 2012 at 1:39 PM, Elliot Poger epo...@chromium.org wrote: Can someone please remind me why IMAGE+TEXT even exists? Wouldn't it be simpler to just mark a test as follows? - IMAGE : allow image failure; go red if there is a text failure - TEXT: allow text failure; go red if there is an image failure - IMAGE TEXT: allow text and/or image failure The distinction is that IMAGE TEXT will allow image, text, or both to fail, thus making transitions among the three generate no events. IMAGE+TEXT says specifically that we expect both to fail and that if one starts passing, someone should do something. (For example, maybe someone checks in a partial rebaseline where they miss the image expectations.) PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] TestExpectations syntax changes, last call (for a while, at least) ...
On Wed, Jun 13, 2012 at 3:58 PM, Darin Adler da...@apple.com wrote: On Jun 13, 2012, at 3:53 PM, Dirk Pranke dpra...@chromium.org wrote: * we use \ (backslash) as a delimiter instead of : and = Seems worse to me. When I see a backslash I assume it’s a line continuation character or a C escape sequence. Gotta admit this one mystifies me too. I must have missed where someone suggested this in the last thread. * As of now the keywords remain all UPPERCASE. I'm happy to change them to all lowercase or mixed case if there's a consensus that such a change is desired. Given the last thread it seems clear there will not be consensus on any outcome of this particular question. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Rename FAIL to DIFF Was (Re: PSA: FAIL test expectation does not encompass MISSING, CRASH, or TIMEOUT)
On Wed, Jun 6, 2012 at 11:28 PM, Ryosuke Niwa rn...@webkit.org wrote: I don't think DIFF is any better. It sounds like it means the output is different than what we wanted, thus it effectively means didn't pass, and one would expect it to match MISSING/CRASH/TIMEOUT as much as one would expect FAIL to. The output being different implies that we have an output, which isn't true when DRT/WTR crashes or times out. Those are outputs as well. Or if you don't like the work output substitute outcome. Personally I'd prefer to resolve this -- if we need to -- by removing FAIL entirely. Being explicit about your expectations isn't a bad thing. Plus, the number of cases that are truly TEXT IMAGE IMAGE+TEXT seems likely to be small. People use FAIL when they don't know what to expect; e.g. adding or rebaselining tests. zit's utterky unreasonable to expect patch authors to add TEXT IMAGE TEXT+IMAGE to every test they're expecting to rebaseline. If you're rebaselining an existing test, presumably the test already has an expectation that matches what it's doing, or you can see what it's doing to write one. If the rebaselining tools can already figure out the right behavior when an expectation is FAIL, why don't we just make them work correctly regardless of what the expectation says? Or work correctly when the expectation is the empty string? Those would let people just write foo.html = and rebaseline which is even easier on them. I also think it's a bad practice to add test expections just to keep bots green. It's much better if the tests started to fail on the waterfall so that people who pay attention to bots can rebaseline them since most people forget about rebaselining tests once their patches are landed. I can't tell what you're arguing here. As far as I know nothing in what I was saying related to the question of the reason why people are adding expectations. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Rename FAIL to DIFF Was (Re: PSA: FAIL test expectation does not encompass MISSING, CRASH, or TIMEOUT)
On Thu, Jun 7, 2012 at 12:33 PM, Ryosuke Niwa rn...@webkit.org wrote: Not if the test was padding. I'm talking about the case where you're modifying WebCore and know that some tests are going to need rebaselines. People have advised in the past that patch authors add failing test expectations to TestExpectations files to avoid turning bots red. I think this is bad advice. When I've been sheriff it seems like people who try this inevitably miss some tests and platforms anyway, get wrong expectations, etc. All this does is add more work for the submitter that is difficult to check ahead of time. We should just advise people to land and then fix (and be around on IRC/notify sheriffs about what's going on). It seems like this is what you were saying as your general statement as well. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Rename FAIL to DIFF Was (Re: PSA: FAIL test expectation does not encompass MISSING, CRASH, or TIMEOUT)
On Wed, Jun 6, 2012 at 10:22 PM, Ryosuke Niwa rn...@webkit.org wrote: Now that everyone knows the problem, I propose to rename FAIL to DIFF. FAIL should mean that the test fails, not that it fails with image, text, or image and text failures. DIFF, on the other hand, has no ambiguity. It can't be interpreted as timeout, crash, or pass but can easily be associated with image and text differences. I don't think DIFF is any better. It sounds like it means the output is different than what we wanted, thus it effectively means didn't pass, and one would expect it to match MISSING/CRASH/TIMEOUT as much as one would expect FAIL to. Personally I'd prefer to resolve this -- if we need to -- by removing FAIL entirely. Being explicit about your expectations isn't a bad thing. Plus, the number of cases that are truly TEXT IMAGE IMAGE+TEXT seems likely to be small. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Simplifying syntax in test_expectations.txt (bug 86691)
On Thu, May 17, 2012 at 9:26 AM, Dimitri Glazkov dglaz...@chromium.orgwrote: I actually quite like the clear delineation between test modifiers and test expectations. Me too. Please please please leave them on opposite sides. All these proposals that try to put them both before the test name are much harder for me to understand. Even dropping the all-caps format for keywords is a loss for me as this differentiation makes it easier for me to scan the line and distinguish the three important pieces of a line: what, where, and how. I find most of the changes that have been proposed so far to be a significant decrease in clarity, except for the idea to change BUGXX### into links (for all bug trackers, not just non-WebKit ones), which I think would be fine; and the idea of moving SLOW, SKIP, and WONTFIX from the left side to the right, as these seem more like expectations about how the test will run than platforms on which the test fails. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using namespace std
On Thu, May 17, 2012 at 12:02 PM, Ryosuke Niwa rn...@webkit.org wrote: It appears to me using fully qualified names (e.g. std::max(~) at call site) is far superior to using directive for individual symbols (e.g. using std::max). It sounds in general like a number of people have been in favor of this, and there hasn't been any opposition. Should we say that the rule for things in std:: is to qualify at the callsite rather than use either form of a using directive? Is there any disagreement with doing that? (I assume we'd leave the existing rules unchanged for other namespaces.) PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Simplifying syntax in test_expectations.txt (bug 86691)
On Thu, May 17, 2012 at 12:19 PM, Ryosuke Niwa rn...@webkit.org wrote: On Thu, May 17, 2012 at 11:54 AM, Maciej Stachowiak m...@apple.com wrote: Perhaps aligning the fields column after the bug URL would improve readability (though it makes things a little harder to edit): We certainly could. We treat \s+ as \s anyway so we have a lot of freedom here. However, aligning things using whitespace in test_expectations.txt is just as problematic in C++ code because the modifiers and platforms can grow: e.g. Pass Image+Text Image Crash Timeout (SnowLeopard Leopard Linux Win7 Debug) Yeah, this is my worry too. None of the options (have ridiculously wide columns, have some rules split across multiple lines, have some rules violate the alignment) seems terribly appealing. If we knew we could restrict the platforms and expectations to a small horizontal space, I think this would work nicely, but to do that we might need to move to cryptic abbreviations :/ PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Simplifying syntax in test_expectations.txt (bug 86691)
On Thu, May 17, 2012 at 1:34 PM, Ojan Vafai o...@chromium.org wrote: 2. Make outcomes optional. If they are left out, then the test is skipped (unless the test is marked SLOW, in which case it's expected to pass). There is no SKIP modifier. I don't think we should do this. It seems very subtle. I'd rather be explicit. I'm OK with the rest of your numbered proposals. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Simplifying syntax in test_expectations.txt (bug 86691)
On Thu, May 17, 2012 at 2:11 PM, Ojan Vafai o...@chromium.org wrote: Oh, I supposed I misread Peter's earlier email as being opposed to this. You didn't misread me. I have the same opinion as you: this would be a change for the worse. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using namespace std
On Tue, May 15, 2012 at 9:31 AM, Darin Adler da...@apple.com wrote: On May 15, 2012, at 5:48 AM, Allan Sandfeld Jensen wrote: To me it seems like an odd practice, so I would like to ask what original rationale behind that style guideline is Adding a list of using declarations like using std::min to the top of each source file would give us another ongoing maintenance job. The list in each file would grow, we’d not remember to remove them when they are no longer needed, and so on. Given how little of std:: we actually use (since WTF is used instead for most things), what about just explicitly qualifying usages with std:: directly? I realize you probably feel like these sorts of qualifications make code less readable, but the impact of that seems minor to me -- the Chromium codebase gets away with doing it and uses std:: far more often than WebKit -- and it avoids the sort of maintenance headaches you mention. It also makes it clearer to a reader when we are actually pulling in types or functions from std:: (which might not always be what we wanted), and avoids any naming conflicts from pulling in the whole std:: namespace (which is apparently the reason for this thread). PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using namespace std
On Tue, May 15, 2012 at 11:37 AM, Ryosuke Niwa rn...@webkit.org wrote: On May 15, 2012 10:53 AM, Peter Kasting pkast...@chromium.org wrote: Given how little of std:: we actually use (since WTF is used instead for most things), what about just explicitly qualifying usages with std:: directly? Can we do that if and only if we have conflicts? Well, I guess we can do anything we want :). It might be nice to have a consistent rule though (much like the current style rule is consistent, if sometimes problematic). An alternative solution is to forward conflicting symbols from std to WTF (i.e. make decisions about namespace in WTF) so that the rest of codebase can simply use the forwarded WTF symbols instead. We could even wrap whatever function we're using with a different name if we wanted. From the bugs linked in the root post of this thread, it looks like the common offenders are std::min(), std::max(), and std::numeric_limits, with a guest appearance by std::make_pair(). It seems like either solution above -- explicitly qualifying usages of these, or forwarding them through WTF -- would probably work. One factor that makes me still favor the explicit qualification is that I've been burned enough by various compilers/environments defining min and max that anything that results in pulling those symbols into the global scope makes me anxious. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Bash scripts should support LF endings only
On Fri, Feb 24, 2012 at 11:13 AM, Ashod Nakashian ashodnakash...@yahoo.comwrote: In short, the issue is that some bash scripts have their svn:eol-style set to native, which is CRLF on Windows. This is causing build failures on some version of cygwin-bash that don't like CR in the scripts. I fixed a number of these cases in the past by doing precisely what you suggested, and got r+ from aroben IIRC. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] WebKit branch to support multiple VMs (e.g., Dart)
On Mon, Dec 5, 2011 at 5:22 PM, Geoffrey Garen gga...@apple.com wrote: We're creating a branch in order to demonstrate that it's useful and that it does not negatively impact hackability or performance. It hadn't occurred to me to view the goals of the WebKit project as applying only to trunk, and not to branches. I'd be interested in hearing from other WebKit engineers: Do you think that the goals of the WebKit project apply to trunk alone, or to branches as well? It sounds from the quote above like Vijay is not trying to change what the goals are, only to obtain an environment where it's possible to demonstrate, by practical experience, that some set of changes is indeed consistent with WebKit's goals. In principle a branch seems appropriate for that to me unless there is widespread agreement that the entire idea of these patches is so contrary to what we want that no one should have ever even thought about doing them in the first place. I'm not sure things are that clear-cut, so seeing a more fleshed-out system might be useful. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using C++ constant pointers (type_name * const) in WebKit
On Mon, Nov 28, 2011 at 1:38 PM, David Kilzer ddkil...@webkit.org wrote: In a discussion on Bug 71921https://bugs.webkit.org/show_bug.cgi?id=71921, Antti, Darin Adler and I started a discussion about using C++ constant pointers in WebKit. Does the WebKit community have a consensus opinion on the matter? It seems like the const in T * const idName is equally useful (or useless) to the const in const T idName. Both are saying that |idName| (as opposed to what it points to, in the first case) is constant. In both cases people are rarely in that habit of using const, especially when |idName| is a local. The same pros and cons seem to apply to both. Therefore I would suggest that if we want to make a rule, we make it apply to both cases. I personally like using const as much as possible (without overstepping logical constness limits), but I also suspect my view is the minority. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Reg. New File Inclusion Style for Webkit
On Sun, Nov 20, 2011 at 1:57 AM, Arunprasad Rajkumar ararunpra...@gmail.com wrote: Why don't we follow chrome style of file inclusions rather than the usual. For example, *#include WebCore/Page/Chrome.h* This will be more convient way of representing the inclusion. Hope it will avoid long compiler inclusion paths and file namespace issues. Here are some pros and cons I can think of: Pros: * If used pervasively, would allow us to greatly trim the compiler include search paths, possibly providing a noticeable build speedup (I have no estimated numbers). * Makes it slightly more obvious to a reader what, precisely, is being depended upon; might make it easier to notice layering violations. Cons: * Would require us to convert the existing codebase (possibly easy with the help of a script, but would at least result in touching all the files) * Generates more change noise when a header is moved around * Could pose problems for ports that need to supply particular headers from some override directory instead of the typical spot. (I'm being vague here because I think this is probably a real issue but I don't actually know the details of enough ports' build setups to be clear.) I would prefer the full-path style myself, especially if there is really a build-time win, but I strongly suspect that a lot of folks would see the benefit here as not worth the cost. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Cleaning up directories in WebCore
On Fri, Nov 18, 2011 at 2:38 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Nov 18, 2011 at 2:27 PM, Adam Barth aba...@webkit.org wrote: We move files around all the time. If we're afraid to move files for these reasons, we'll be stuck with bad organizational choices we made in the distant past. WebKit's philosophy is to think of the future as large, so the future benefits of these sorts of changes often outweigh the transition costs. Sure. But the question is whether doing this re-org will benefit us in the end or not. I don't think we can always assume any re-org is good. Consider it this way: if the re-org doesn't benefit us, we shouldn't do it regardless of these reasons. If it does, these reasons should not stop us. Therefore, I don't think we need to spend a lot of time thinking about them -- it is enough to acknowledge that re-orgs have nonzero cost in general. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] It's time to remove the Haiku port
On Thu, Nov 3, 2011 at 7:32 PM, Ryan Leavengood leaveng...@gmail.comwrote: The primary problem with our port right now is some of the developers made the choice (which I objected to) to make our own Subversion repo containing the WebKit code to make it easier (in their eyes) to develop the port. Where can we go from here? What does the WebKit project need to see from the Haiku maintainers to have us be a supported port again? It seems like either you should be a private fork/port, or a public upstreamed one, but not both. If you want to live in the upstream tree, I think most of your development work should land quickly in public. If you want to work in a separate repository without landing changes back upstream quickly*, I think it's better to stay private until the point at which that changes. *At most I don't think your private repo should be more than a couple weeks off the public repo. Based on our experience with the Chromium port I think it would be far easier on your group to be public and do development directly in the public tree. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Gamepad API [Was: New feature flag proposal: Joystick API]
On Wed, Oct 12, 2011 at 3:58 PM, Alexey Proskuryakov a...@webkit.org wrote: Quoting what I actually said in the bug, I don't think that we should accept an implementation of a spec that's so immature that it doesn't even have a meaningful name. That the name is not meaningful, and that this is a sign of too great of spec immaturity, are both judgment calls. I don't happen to agree with your judgment in either case (not that my own judgment is objectively better). There's been a noteworthy lack of good alternative ideas for the name (joystick has similar issues to gamepad, whereas input device or similar are clearly far too broad for what the spec covers; in the end I agree with Darin that the name is fine overall), and in that context I'm not sure the name is evidence for spec immaturity. We had consensus on webkit-dev that the name was bad, yet a patch was again posted for review in Bugzilla. Consensus seems somewhat strong. Looking over the emails sent, you expressed some concern, but there was not a ton of discussion beyond that. I read the tone of the thread as generally OK with things. (If that hadn't been the case I would have sent this email earlier.) As Adam notes, we should be confident of the name before we turn it on by default, but I don't think that should block development, and I also think it would behoove people unhappy with the name to offer some clearly-superior proposals. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] New feature flag proposal: Joystick API
On Thu, Oct 6, 2011 at 11:50 AM, Alexey Proskuryakov a...@webkit.org wrote: I don't want to take a particularly strong stance against Joystick/Gamepad specifically, but I see codifying input device fragmentation in Web specs and APIs as a move in a wrong direction. Why are you convinced there is input device fragmentation here? My understanding is that the spec as proposed already handles multi-axis digital and analog controls so things you mentioned earlier like wheels and 3d mice would all just work along with gamepads and joysticks. I could be misinformed, but it would be nice for you to be concrete about precisely what parts of the spec are currently problematic. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] New feature flag proposal: Joystick API
On Thu, Oct 6, 2011 at 1:42 PM, Alexey Proskuryakov a...@webkit.org wrote: 06.10.2011, в 13:23, Peter Kasting написал(а): Why are you convinced there is input device fragmentation here? My understanding is that the spec as proposed already handles multi-axis digital and analog controls so things you mentioned earlier like wheels and 3d mice would all just work along with gamepads and joysticks. If accurate, that could be a great resolution to the dispute. I do not know enough about these devices to say whether wheels and 3d mice (as an example) are covered. The spechttp://dvcs.w3.org/hg/webevents/raw-file/default/gamepad.html explicitly states that it's about gamepad devices (also known as joysticks). Spec section 3 states: This allows support for devices common to current gaming systems including gamepads, directional pads, joysticks, wheels, pedals, accelerometers. It excludes support for more complex devices that do motion sensing, depth sensing, video analysis, gesture recognition, and so on. It seems to me that this is an appropriate line to draw. Devices like the Kinect seem categorically different from most classical game input devices and I think it's reasonable to exclude them from the first version of the spec, as long as it covers the other types of things fairly well. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Fastest image format
On Tue, Oct 4, 2011 at 2:22 AM, Joe LaFritte joelafri...@yahoo.fr wrote: What is the fastest image format for wekbit ? I mean which image format (jpg, png, gif, etc.) is decoded and displayed fastest than the other ones ? That likely depends on the image, the decoder, and the system in question. For example, BMPs can be stored as uncompressed data, which requires no transformation to display onscreen, but means the source data is much larger so the image takes longer to download. Similarly, the other image formats can generally support lots of different compression techniques and levels that have large effects on image size and decode time, especially if the decoders in question have optimized inner loops for your chip. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Change to style guideline: should use type instead of type* for out arguments
On Tue, Oct 4, 2011 at 2:06 PM, Ryosuke Niwa rn...@webkit.org wrote: In my understanding, we use pass by reference for out arguments when they have to be modified in callees. I had not heard this. Personally I weakly prefer pointers to non-const refs for outparams, but if there is convention the other way that's fine. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Mouse Lock API
On Wed, Sep 21, 2011 at 12:05 PM, Alexey Proskuryakov a...@webkit.org wrote: Anyway, I'm not sure if we already agreed that mouse lock is only desirable in full screen. I think that the spec has the goal of enabling it in browser windows. Indeed, this is explicitly a use case we want to allow. It's reasonable to want to play many mouse-lock-requiring games (Quake, Starcraft, etc.) in non-fullscreen mode, e.g. so one can still keep some other critical display open elsewhere on the screen. PK ___ 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 Mon, Sep 12, 2011 at 1:56 PM, David Levin le...@chromium.org wrote: On Mon, Sep 12, 2011 at 1:48 PM, Peter Kasting pkast...@chromium.orgwrote: In particular, if we have pixel tests that don't need to be pixel tests at all, or font rendering differences due to explanatory text that could be moved to an HTML comment inside the test itself, we can obviate the need for port-specific baselines in many of those cases (and eliminate more baselines from ports already in the tree, and reduce the burden on other ports that want to run pixel tests, and reduce the maintenance cost of changing the tests). Are y'all suggesting that efl port should do these items (converting pixel tests to non-pixel tests) before committing their baselines? I don't think it's fair to force the EFL folks to do all this. I do think all ports would benefit from it and all ports should be spending some effort on this kind of thing. Inasmuch as there is an opportunity here for doing this work to minimize the landing cost for the EFL baselines, I'm raising this so that those folks can keep the option in mind and make use of it opportunistically. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Color profiles in expected.png files
On Fri, Sep 9, 2011 at 7:21 PM, Simon Fraser simon.fra...@apple.com wrote: I think we should be consistent about color profiles, and in a way that doesn't break pixel tests. Does anyone want to vote one way or the other? I suspect no profiles is the only way that will work across both ports that handle embedded color profiles and ports that do not. (AFAIK some Chromium ports currently do not.) PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Building Chromium port of WebKit on Windows
The way I make this work is to set up a full Chromium checkout with a trunk (rather than DEPS-controlled) WebKit checkout. (There are some instructions for this in the Chromium developer pages.) Then I can use VS2008 to build and test whatever I want. And I use Cygwin. I don't know much about trying to build the Chromium port with only a WebKit (not Chrome) checkout, especially under Cygwin. I don't know how many people try to make that work. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Building Chromium port of WebKit on Windows
On Thu, Aug 25, 2011 at 10:07 AM, Mikhail Naganov mnaga...@chromium.orgwrote: I'm not sure we are on the same page. I'm talking about Chromium port of WebKit, where Chromium checkout is _inside_ WebKit's Source/WebKit/chromium, as opposed to when you have full WebKit checkout inside Chromium's third_party. Yes, we're saying always use the latter, as it lets you test the same things and it's much more well-supported and tested :) PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Building Chromium port of WebKit on Windows
On Thu, Aug 25, 2011 at 10:31 AM, Dimitri Glazkov dglaz...@chromium.orgwrote: FWIW, I use the former exclusively on Mac. Yes, please regard my comments as only applying to Windows. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Switching away from integers for layout
On Thu, Jun 23, 2011 at 11:46 AM, Levi Weintraub le...@chromium.org wrote: To address this we plan to convert the rendering tree to float to allow for better zooming and scaling support. Furthermore this could be expanded to support sub-pixel layout and positioning which in turn would allow higher precision layout when zoomed. We’re the only rendering engine that hasn’t yet made this change. I thought Gecko eschewed floats in favor of some sort of more complex fixed-point-esque system. Am I mistaken? I looked on the metabug to see if Hyatt had made comments, but didn't see any. Do you have feedback from him yet? PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] 194 bugs in pending-commit
On Fri, Jun 17, 2011 at 10:56 PM, Adam Barth aba...@webkit.org wrote: 2) Mark the patch as obsolete / clear the review flag if we're not going to land the patch. Does the slash mean do both? I have https://bugs.webkit.org/show_bug.cgi?id=47036 on that list and the only r+ed patch on it is already marked obsolete. PK ___ 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 9, 2011 at 2:49 AM, Maciej Stachowiak m...@apple.com wrote: I'm not really convinced that casting away const from a return value is intrinsically safer than casting away const from this. Allowing the caller to mutate the return value is fine because the caller had a non-const |this| to begin with. We're not making anything less const-safe. Casting away const on |this|, OTOH, allows you to mutate objects even when you never had permission to begin with. Much different. In any case, my intent is to proceed as Darin and I discussed. PK ___ 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 9, 2011 at 3:51 PM, Maciej Stachowiak m...@apple.com wrote: In principle, the return value could have been retrieved from a container that the immediate callee only has a const reference to. So then casting away const on the return value would be a hazard. You're right; if the implementation of the const function accesses other objects besides |this|, it might be better to re-implement the non-const function rather than simply casting, so the compiler can at least check those accesses. This implies that in general we'd want to limit function pairs to accessors with fairly simple implementations, and only make exceptions where there's real value, lest the maintenance burden and bug surface outweigh any gain from providing caller const convenience. I'll try to keep this in mind before I try posting any patches that split accessors into pairs. PK ___ 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, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak m...@apple.com wrote: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. There are a surprising number of places that actually do const traversals. Simply making all these accessors non-const will require removing a lot of valid const usage from the existing code. I'm really uncomfortable with that. 2) We like to do things incrementally and fixing bad use of const before adding good use of const seems like this is a logical way to split the work and make it less of a megapatch. Incremental fixes are absolutely the way to go. Reviewing megapatches sucks and it's hard to catch subtle bugs like you changed this function to be not const, but there's no reason to do that. Perhaps a split that avoids removing existing, valid const usage would be to first change few (or no) function signatures, and simply modify caller code to be more consistent about type declarations. This would mean converting some callers from Node* to const Node* when they're doing true const traversals, and some the opposite direction when they're not. The goal would be to make eventual API changes a no-op in terms of caller effect. It'd be easy to make these sorts of patches arbitrarily small as well. (As a separate technical comment, I think it may be better to have the const versions call non-const versions (casting away const on this), because the non-const versions are likely to be called much more often so it's helpful to have fewer levels of indirection to wade through before seeing the real code, e.g. when inspecting code or debugging.) I totally agree that these sorts of indirections are suboptimal (especially for common cases). The particular idea you propose isn't safe because there's no protection against the non-const impl actually causing side effects. Even if current implementations don't, it's easy to add a subtle bug like this in the future. And while compilers won't enforce this perfectly, they'll do a better and better job (better than nothing, for sure) as we change more APIs to enforce logical constness. (I hate to keep referencing it, but the end of Effective C++ Item 3 directly addresses this implementation idea in more detail. That whole section is really worth reading for anyone deeply interested in this issue.) However, I think we should at least try to limit the number of accessor pairs to only those cases which really need them. What about this plan: * I'll post a patch that just addresses the caller-side constness issues I've found from my work so far. * Then in my local checkout I'll start trying to see which accessor pairs I can collapse back to one accessor, be it const or non-const. I'll post patches to make any necessary caller-side changes here, too. * Finally I'll post a patch to change method signatures and add accessor pairs where necessary. * Then rinse and repeat with another class, e.g. Element. I'll go ahead and file a bug and start posting real diffs to look at unless this plan has fatal flaws. PK ___ 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 Wed, Jun 8, 2011 at 11:51 AM, Darin Adler da...@apple.com wrote: On Jun 8, 2011, at 11:48 AM, Peter Kasting wrote: On Tue, Jun 7, 2011 at 11:18 PM, Maciej Stachowiak m...@apple.com wrote: 1) We definitely have consensus to fix the broken non-logically-const accessors by making them non-const; consensus on also adding const accessors is less clear. There are a surprising number of places that actually do const traversals. Simply making all these accessors non-const will require removing a lot of valid const usage from the existing code. I'm really uncomfortable with that. I thought you did it already locally. You mentioned that you decided for many member functions that the right thing was to remove const. I suggested you land those changes first, before making the other changes. Are we talking about the same thing? Maybe you think Maciej is asking for something he’s not. Maybe I got confused. Some accessors cannot be const at all (IMO), like the ones that update layout before returning the desired value. Other accessors, e.g. parentNode(), don't themselves do anything non-const and so they could theoretically be valid as const and non-const versions. What I thought Maciej was saying was that we should remove const on all the existing accessors, in both categories, which sounded different than what you were saying (which I read as remove const on the accessors in the first category). I'm perfectly happy removing const from accessors in the first category. I can post a change that does that before going any further. PK ___ 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 Wed, Jun 8, 2011 at 11:59 AM, Darin Adler da...@apple.com wrote: On Jun 8, 2011, at 11:56 AM, Peter Kasting wrote: I'm perfectly happy removing const from accessors in the first category. I can post a change that does that before going any further. Yes, I believe that’s what both Maciej and I were suggesting. Great. I filed https://bugs.webkit.org/show_bug.cgi?id=62302 about this and will proceed there. PK ___ 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 Fri, Jun 3, 2011 at 5:59 PM, Darin Adler da...@apple.com wrote: On Jun 3, 2011, at 5:46 PM, Peter Kasting wrote: From the perspective of Node itself, I'm not sure why this would be a big task. There shouldn't be any const accessors that return non-const pointers. Simply removing the const qualifier on all the above accessors would make things correct, at the expense of possibly making it difficult to use a const Node*. Maybe you can give it a try and report back. I think you’ll find that this is an enormous task. I now have a local patch which mostly fixes const-correctness of the API declared in Node.h, which I've tested only for Chromium Windows (and only WebCore, no other bits). Before I post it for review somewhere I'd like to get feedback on how to proceed. By fixes, I mean that in most cases, I converted any A* Node::getA() const accessor to a pair of accessors, const A* Node::getA() const and A* Node::getA(). This should flush out people who are trying to perform non-const operations on const pointers, without preventing existing valid usage of const pointers, at the cost of adding a lot of extra accessors that aren't necessarily called. It doesn't guarantee transitive const correctness on other classes besides Node which may have similar accessors, but that's something of an advantage here because it allows me to write a patch to improve Node without having to fix all accessors across all WebCore. For all these pairs of accessors I used the trick from Effective C++ of having the non-const version call the const version and then cast away const on the return type, which looks a little unwieldy at first but guarantees the two accessors cannot get out of step in the future. I avoided fixing Node::scriptExecutionContext() because that's overridden in a zillion different places and I didn't want to touch all of those. I did fix a number of other accessors in other files where they were transitively affected by the changes above. Finally, I made a few decisions on functions to simply make non-const. For example, any accessor which called Document::updateLayoutIgnorePendingStylesheets() was made non-const because that really does have a lot of side-effects. According to jamesr this may be a good thing anyway because apparently we've had bugs (including security bugs) in the past where people call some innocuous-looking const accessor like Node::isContentEditable() and then they end up causing unintentional changes. I'm happy to post this for review somewhere. However, I wonder if first I should try to reduce the pairs of accessors I created above back down to one accessor wherever possible, based on which version is actually called. This would reduce API sprawl at the cost of making the actual APIs somewhat arbitrarily const or not. This might also discourage future users of these classes from re-adding the partner of an existing accessor, in the same way that writing classes without any const accessors encourages people not to ever try to use them via const pointers. Assuming we want this, I would also like to know how we'd like to proceed after this patch. I'm happy to clean up major classes like Element and Document one at a time, but it'd be nice if we settled on a consistent style policy and, if we want to fix a lot of the existing code, if more people than me could help (or if we could automate some things with tooling). PK ___ 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 Fri, Jun 3, 2011 at 5:27 PM, Darin Adler da...@apple.com wrote: From a const Node* you can get: - a non-const pointer to a parent, sibling, or child - a non-const pointer to the document - a non-const pointer to the renderer - a non-const pointer to the style - a non-const pointer to various shadow-related ancestors and hosts That’s one problem. Extending the const-ness of the node to mean constness of everything you can get through all these pointers would be a big task, and it’s not clear it would be worthwhile. From the perspective of Node itself, I'm not sure why this would be a big task. There shouldn't be any const accessors that return non-const pointers. Simply removing the const qualifier on all the above accessors would make things correct, at the expense of possibly making it difficult to use a const Node*. Adding const versions of the accessors where necessary, which themselves vend const pointers, is not significantly harder. The only way this is a big task is if there are callers that make significant use of const pointers to do non-const actions. Then we need to clean these up. But if they're doing non-const actions, then the cleanup is simply to make them use non-const pointers. Further, from the document you can get to the frame and things like the selection controller. Similarly, you shouldn't be able to get non-const pointers to the frame or selection controller from a const document pointer. Experience with the C++ standard library taught me that a constant pointer to something within a collection is a difficult concept to model with const. The key example here is the std::vector::erase function. It seems illogical that you can’t call erase on a const_iterator, because the iterator is identifying the location within the vector, not whether you have permission to modify the vector. You’re not modifying something through the iterator. Whether or not it's reasonable to have non-const functions on containers, which modify only the container and not the elements within it, take const pointers to elements, seems like a separable question from the rest of the issues with using const correctly. I happen to find the standard library's behavior here more reasonable than what you suggest, but regardless, all of the other cases above could certainly be handled in a clearly correct fashion. In other words, I don't find this problem a compelling reason to discard the entire idea of constness when it comes to objects which participate in tree/graph relationships. PK ___ 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
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] Do we have a style preference about const member functions?
On Mon, May 30, 2011 at 11:32 PM, Maciej Stachowiak m...@apple.com wrote: A linked list node or tree node could useful have const methods, which give only const pointers/references to other nodes. If there is a reason const references to DOM nodes or renew objects are not useful, it is not due to the object graph participation itself, in my opinion. Indeed. The rule of thumb I use is that a const member function should only return const-ref or pointer-to-const objects (whether as return values or outparams). This helps ensure that the method is logically const, not just physically const, by preventing callers from using const methods to gain handles they can use to modify supposedly-const objects. It so happens that objects in trees/graphs tend to have functions that return pointers to other objects much more frequently than do independent data holders. Thus Geoff's rule ends up approximating my rule, but is not equivalent. As to use of const in general, I would prefer to see more of it rather than less, _assuming it is used only for logical constness_. See Scott Meyers' Effective C++, item 3, for rationale. PK ___ 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:00 AM, Maciej Stachowiak m...@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! Because this is subtle, and can't be completely checked by a compiler, people can often get it wrong. Used correctly, though, const is a powerful tool for enforcing correct API usage, allowing optimizations, and informing readers of a class' contract and functionality. Reviewers should understand and consider these sorts of details. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Large Source Reorganizations By External WebKit Ports
On Thu, May 19, 2011 at 9:09 AM, Charles Pritchard ch...@jumis.com wrote: I think Brent's question to the list may have some merit if looked at from a different perspective. Let me try it... Peter: Are there any lessons learned about that process Chromium went through? Darin Fisher shared most of the valuable information here. In Chromium's case, we were forced to fork due to secrecy constraints during our initial development -- something which none of us liked from an engineering perspective, but which I suspect tends to apply to a large number of ports. As a result, we became focused on making changes in a minimally invasive way, to lower the cost of merging. The consequence of this was a much higher later upstreaming cost, as the minimally invasive way and the right way are frequently not the same. Additionally, we froze our fork before our first public release, and didn't unfreeze until almost ten months later. This added another burden to the unforking process. It took something like a year of time and a large amount of engineering effort to unfork. However, the alternative of staying forked forever incurs a perpetual cost on the development team, in addition to being worse for WebKit as a whole (due to the upstream codebase not truly reflecting ports' needs and other ports not benefitting from each port's work). As a result, I think any company that intends to have a long-lived product based on WebKit is making a grave mistake if they don't dedicate significant engineering time to unforking, costly as that is. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Large Source Reorganizations By External WebKit Ports
On Wed, May 18, 2011 at 12:36 PM, Brent Fulgham bfulg...@webkit.org wrote: Google used this same approach with their Chromium port, the side effects of which find us in year two (or three?) of the effort to merge those changes back into the core WebKit archive. Um, what? The Chromium port is fully upstreamed and has been for some time. I'm not sure what you're saying here. We are not forked and in fact have no support for building Chromium with anything other than upstream WebKit. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Platform LayoutTests are out of control
Hi Brent, In a past thread, I noted that we could do a couple of things to reduce platform-specific results, and the overall size of layout test results. In order of increasing difficulty: * Convert pixel tests to dumpAsText tests when pixel output is unnecessary (merely requires adding a command to the test file) * Avoid printing text onscreen in pixel tests unless critical to the tests -- use comments in the code instead * Convert tests to reftests These got general support, but no one actively interested in going through our existing tests to see where they could be applied. Would you be interested in helping organize such an effort? :) PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Platform LayoutTests are out of control
On Wed, Apr 20, 2011 at 10:46 PM, Ryosuke Niwa ryosuke.n...@gmail.comwrote: On Wed, Apr 20, 2011 at 10:41 PM, Peter Kasting pkast...@google.comwrote: * Convert tests to reftests I don't think we should do this until all ports start using new-run-webkit-tests on their bots. It's the most labor-intensive choice anyway, so the lowest-hanging fruit is in the other two changes for sure. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 1:08 PM, Peter Kasting pkast...@google.com wrote: On Fri, Mar 25, 2011 at 1:03 PM, Mark Rowe mr...@apple.com wrote: Safari 5.0.3 will always report a build version of 533.19.4. Safari 5.0.4 will always report a build version of 533.20.27. You already used the former in your before examples. Ideally, I would like the Safari-on-Windows and Safari-on-Mac strings from a trunk build as of around the time my change landed, which I'd use as the identical before and after strings, and only change the portions actually affected by the UA changes that landed. Since this is slated for publication this morning, I checked with dglazkov and updated my draft to use the current UA strings from a ToT WebKit build. Hopefully now all the version numbers are sane and consistent. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] UA string changes blog draft
Hello WebKit developers, I've created a draft blog post at http://www.webkit.org/blog/?p=1580 about the recent changes I and others have made to the UA string. I'm interested in any feedback you might have. I've written a similar blog post, but focused on Chrome and aimed at a wider audience, that the Google folks intend to cross-post on both the Chromium and Google Webmaster Central blogs on Monday morning. I'd like to publish this draft at a similar time, barring objections. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 10:50 AM, Peter Kasting pkast...@google.com wrote: I've created a draft blog post at http://www.webkit.org/blog/?p=1580 about the recent changes I and others have made to the UA string. I'm interested in any feedback you might have. Note, since this is a draft, you need to log in to blog.webkit.org to see it (creating an account is simple and free). For those who haven't logged in or don't wish to, here's the current fulltext. PK --- UA String Changes On WebKit Trunk http://www.webkit.org/blog/?p=1580Posted by *Peter Kasting* on Friday, March 25th, 2011 at 10:44 am Recently some changes to the UA string (tracked by https://bugs.webkit.org/show_bug.cgi?id=54556) have landed. These changes are designed to add UA string detail, remove redundancy, and increase compatibility with Internet Explorer, and are happening in conjunction with similar changes in Firefox 4 (which you can read about at http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/). Here’s a few sample pre-change UA strings: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_7; en-us) AppleWebKit/534.16+ (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4 Here’s some sample post-change UA strings: Mozilla/5.0 (Windows NT 6.0; WOW64) AppleWebKit/534.24 (KHTML, like Gecko) Version/5.0.3 Safari/534.24 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_7; en-us) AppleWebKit/534.24 (KHTML, like Gecko) Version/5.0.3 Safari/534.24 In detail, the differences are as follows: 1. On Windows, the initial “Windows;” platform identifier has been removed. This was redundant with the subsequent OS version identifier, and is more compatible with Internet Explorer, whose UA string doesn’t have this initial token. 2. The “U” SSL encryption strength token has been removed. This token dates from more than a decade ago, when U.S. export laws limited the encryption strength that could be built into software shipped to various other countries; the valid values are ”U” (for “USA” 128-bit encryption support), “I” (for “International” 40-bit encryption support), and “N“ (for “None”, no encryption support). These days, it’s unusual to ship without 128-bit SSL support everywhere; ports can add “I” or “N” if necessary. 3. On 64-bit versions of Windows, tokens have been added after the OS version. 32-bit builds running on 64-bit Windows have added “WOW64“. (”WOW64″ stands for “Windows 32-bit On Windows 64-bit” and is the name Microsoft gives its 32-bit compatibility subsystem.) 64-bit native builds use “Win64; x64” for x64-based processors and “Win64; IA64” for Itanium systems. These tokens are useful for sites that need to provide download links for native executables, and match what Internet Explorer uses. 4. The locale has been removed. Web authors who want to know what languages a browser supports should use the HTTP Accept-Language header instead, which can supply multiple locales. 5. Windows CE builds should report the OS version slightly more accurately (e.g. “Windows CE 5.1” instead of “Windows CE 5.x” or “Windows 5.1“). Google intends to ship Chrome 11 with these changes, assuming they don’t cause major web compatibility problems, in order to get them into place as soon as possible after the Firefox 4 and IE 9 launches, and is already testing them in Chrome Dev and Beta channel builds. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 11:05 AM, Patrick R. Gansterer par...@paroga.comwrote: If you take a look at http://trac.webkit.org/browser/trunk/Source/WebKit/wince/WebCoreSupport/FrameLoaderClientWinCE.cpp#L57I’m not sure if point 5 is correct. ;-) The WinCE port has still my initial UA string for testing. My comments applied to the only WinCE-specific strings I had found, which were in the Qt port. I'll clarify that point. It should be easy for this code to do something like what the WebKit/win and WebKit2/win UA string code does, based on calling windowsVersionForUAString(), which should give correct output on Windows CE. You might want to file a bug to switch to that, if this port isn't dead. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
I've incorporated all the existing feedback into the draft. Feel free to take another look. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 11:54 AM, David Levin le...@google.com wrote: The blog post begs the question made me wonder. Why was Macintosh; kept when it is redundant with Intel Mac OS X 10_6_7? The reasoning seem analogous to what was given for why Windows; was removed. Unlike Windows, the string Macintosh does not otherwise occur in the OS version. So sites looking for the token Macintosh to detect Macs will all break. Also, the main reason for both Gecko and WebKit to remove Windows was to match IE, which isn't a factor in the Mac UA string. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 11:44 AM, Peter Kasting pkast...@google.com wrote: I've incorporated all the existing feedback into the draft. Feel free to take another look. Since some folks seem to be unable to see the draft even while logged in, here's the new fulltext. PK --- User Agent String Changes On WebKit Trunkhttp://www.webkit.org/blog/?p=1580Posted by *Peter Kasting* on Friday, March 25th, 2011 at 11:44 am Recently some changes to the User Agent (UA) stringhttps://bugs.webkit.org/show_bug.cgi?id=54556 have landed. These changes are designed to add UA string detail, remove redundancy, and increase compatibility with Internet Explorer, and are happening in conjunction with similar changes in Firefox 4http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/ . Here are a few sample pre-change UA strings: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_7; en-us) AppleWebKit/534.16+ (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4 Here are the equivalent post-change UA strings: Mozilla/5.0 (Windows NT 6.0; WOW64) AppleWebKit/534.24 (KHTML, like Gecko) Version/5.0.3 Safari/534.24 Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_7) AppleWebKit/534.24 (KHTML, like Gecko) Version/5.0.3 Safari/534.24 In detail, the differences are as follows: 1. On Windows, the initial “Windows;” platform identifier has been removed. This was redundant with the subsequent OS version identifier, and is more compatible with Internet Explorer, whose UA string doesn’t have this initial token. 2. The “U” SSL encryption strength token has been removed. This token dates from more than a decade ago, when U.S. export laws limited the encryption strength that could be built into software shipped to various other countries; the valid values are “U” (for “USA” 128-bit encryption support), “I” (for “International” 40-bit encryption support), and “N” (for “None”, no encryption support). These days, it’s unusual to ship without 128-bit SSL support everywhere; ports can add “I” or “N” if necessary. 3. On 64-bit versions of Windows, tokens have been added after the OS version. 32-bit builds running on 64-bit Windows have added “WOW64”. (“WOW64” stands for “Windows 32-bit On Windows 64-bit” and is the name Microsoft gives its 32-bit compatibility subsystem.) 64-bit native builds use “Win64; x64” for x64-based processors and “Win64; IA64” for Itanium systems. These tokens are useful for sites that need to provide download links for native executables, and match what Internet Explorer uses. 4. The locale has been removed. Web authors who want to know what languages a browser supports should use the HTTP Accept-Language header instead, which can supply multiple locales. 5. Windows CE builds of Qt-based ports should report the OS version slightly more accurately (e.g. “Windows CE 5.1” instead of “Windows CE 5.x” or “Windows 5.1”). As various ports ship these changes, you might notice web compatibility problems. If so, please point webmasters to this post, and/or file bugs in the bug tracker http://bugs.webkit.org/. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 12:54 PM, Mark Rowe mr...@apple.com wrote: Is there some reason why these examples use manufactured Safari build numbers? It's implausible that a version of Safari with a build number of 534.24 would ever claim to be version 5.0.3. Sorry, I wasn't sure what the right numbers were. What would be a more accurate number? I'd be happy to change it. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] UA string changes blog draft
On Fri, Mar 25, 2011 at 1:03 PM, Mark Rowe mr...@apple.com wrote: On 2011-03-25, at 12:56, Peter Kasting wrote: On Fri, Mar 25, 2011 at 12:54 PM, Mark Rowe mr...@apple.com wrote: Is there some reason why these examples use manufactured Safari build numbers? It's implausible that a version of Safari with a build number of 534.24 would ever claim to be version 5.0.3. Sorry, I wasn't sure what the right numbers were. What would be a more accurate number? I'd be happy to change it. I'm not sure what you're asking here. Safari 5.0.3 will always report a build version of 533.19.4. Safari 5.0.4 will always report a build version of 533.20.27. You already used the former in your before examples. You have also been inconsistent about the WebKit version number. In the before version for the Mac user agent string you've included the + suffix that indicates an engineering build (from a local build or nightly). This isn't present in the Windows before version or either of the after versions. Since that component isn't relevant to the point you're trying to make it seems like it would be preferable for it to be consistent between examples. Ideally, I would like the Safari-on-Windows and Safari-on-Mac strings from a trunk build as of around the time my change landed, which I'd use as the identical before and after strings, and only change the portions actually affected by the UA changes that landed. I don't have the ability to build either of those myself right now, so what you're seeing is a combination of internet research, inference from Chromium builds, and mistakes in copy and pasting. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Build system update
On Wed, Mar 23, 2011 at 2:08 PM, Adam Barth aba...@webkit.org wrote: Indeed. I suspect (2) and (3) are worth doing regardless. AFAIK, gyp currently always regenerates everything and then compares the new versions to the old to see if it actually needs to touch the files on disk. This seems safe but slow. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Every build is crashing
On Tue, Feb 15, 2011 at 11:23 AM, Adam Barth aba...@webkit.org wrote: This has been discussed on IRC when it started - everyone present seemed to be OK with that, and didn't want to revert. Please feel free to revert, of course. I must have missed that discussion. Maybe we should note these things in the #webkit channel topic? FWIW, there wasn't really much chatter -- I noted that I'd found a crash in a Chromium build, Alexey told me to feel free to revert if needed, and I said that I already had a crash suppression in for Chromium and a bug on file, so I wasn't too worried. I didn't know other builds were crashing, and I don't think many other people were around at the time, so it's possible other folks would have wanted to revert then. Sorry! In any case, it looks to have been cleared up. Yep, thanks to Antti, Andreas and anyone else involved for the quick fix. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Aligning User Agent Header changes
On Thu, Feb 10, 2011 at 7:32 AM, laszlo.1.gom...@nokia.com wrote: QtWebKit is considering dropping the language tag part of the User Agent string - following Firefox ( http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/). I think the WebKit community did a reasonable job at aligning the User Agent string for WebKit based products and I was hoping that we could continue this good habit, so I was wondering what other ports/products would think of this change. Several of us on the Chromium side have mentioned this to each other. We're definitely interested in simplifying the UA string, especially if it can be done in concord with other browsers. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Web Inspector blog post draft
On Wed, Feb 9, 2011 at 4:32 AM, Alexander Pavlov apav...@chromium.orgwrote: I have put together a Web Inspector blog post draft ( http://webkit.org/blog/?p=1463) concerning the latest style editing improvements. Please speak up if you think something should be changed, added, or removed. I get an error page at this URL. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] precompiled headers
My understanding is that it is supposed to be possible to build all ports without PCH support. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Sun, Jan 30, 2011 at 4:47 PM, Maciej Stachowiak m...@apple.com wrote: Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call well-commented, by which they mean lots of comments. I agree that the comments you pointed out are pretty unhelpful. I tried to emphasize already that silly comments that just restate the next line of code are not at all what I mean by well commented, and that what I am interested in are comments about subtle but crucial details (e.g. complex ownership rules) or comments that sum up a huge swath of source code (e.g. a class-level comment that covers the critical high-level points the class is responsible for). I honestly don't think there is much disagreement about what kinds of comments are unhelpful. I think the disagreement here comes from past experience, where some people have mostly experienced low-quality and out-of-date comments and are justifiably uninterested in repeating that, and others have been helped by high-quality comments in complex codebases and want to see more of that in WebKit. It seems like the best rule of thumb would be that reviewers should look at any added comments and judge whether the comments are really valuable. I don't think we need to (or should) globally frown on comments -- just on bad comments. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Using Visual Studio 2010 for Apple's Windows WebKit port
On Mon, Jan 31, 2011 at 12:30 PM, Adam Roben aro...@apple.com wrote: Please let me (and the list) know if this change will cause you trouble, and if there's something we can do to make the transition easier. This may make life hard on Chromium as right now we don't support building with VS2010. We are working on adding support (I think http://crbug.com/25628 may be the closest bug). I'm not sure what time frame that will happen in. I don't think we have any plans to drop VS2005 support. If WebKit drops support it may force us to do so as well. In both cases above we're not so much impacted by vcproj/sln changes (since we use GYP to construct our vcproj and sln files) as we are by code changes, e.g. code that requires VS2010 to build correctly. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak m...@apple.com wrote: On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote: I think people who favor comments tend to produce a lot of exactly this kind of comment. Except in some cases its verbose multiline comments that exceed the number of actual lines of code. Here's some more files with many comments that are better deleted or replaced with refactoring: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp Oh good, files I know something about :) To pick just one specific example: // ICOs always begin with a 2-byte 0 followed by a 2-byte 1. // CURs begin with 2-byte 0 followed by 2-byte 2. if (!memcmp(contents, \x00\x00\x01\x00, 4) || !memcmp(contents, \x00\x00\x02\x00, 4)) This would be more readable if the comments were deleted and the memcmps were replaced with calls to inline functions named hasICOMagicNumber/hasCURMagicNumber or the like. Or just omitted altogether. I don't think the comments here add much. (They predate my involvement; they actually come from hyatt in 2006, and in fairness to him, this code at that time was very much throw in something quick that works to get a Windows port up and running.) Your choice of files is interesting otherwise though. Aside from those 2006-era comments in create(), there are only three other comments in ImageDecoder.cpp, two of which seem worth having to me. In ImageDecoder.h, many function declarations are commented, most to explain ownership details, caution users of functionality that needs to match in multiple places, or give more context to why callers would use the function. There are definitely some comments lying around here that aren't terribly useful, but the majority of these have been helpful when tracing through various different image decoding bugs, especially memory errors triggered by our heuristic that throws away image data for large images sometimes, or when refreshing my memory on what this code does when I come back to it after a year of doing something else. So I don't agree that many comments here (if many means the majority) are unhelpful. I guess, then, we do disagree about what types of comments are useful. I would go further than that. Even if a comment is valuable, the reviewer (and the patch author) should think about whether what it says could be expressed in source code instead, with some refactoring. Comments should be a last resort for making code clear. I do agree that they have their place, but I think that place is fairly rare. I think we are probably going to remain in disagreement, then. In my opinion, this is fine if you already know the code in question in detail, but I agree with Simon that this level of strictness is a barrier to learning new code and contributes to a privileged few sort of view of code ownership. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
This thread has probably gone the way of all webkit-dev threads on comments or ChangeLog files -- people's opinions vary, it turns into a bikeshed, and nothing really changes about how we code. Repeat in a year. w.r.t. ImageDecoder specifically, as I mentioned before I do agree that there are some comments that are either worthless or partially so, and I'll try and post some cleanup for this header on https://bugs.webkit.org/show_bug.cgi?id=53455 . PK P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Thu, Jan 27, 2011 at 4:27 PM, Simon Fraser simon.fra...@apple.comwrote: I think we have a distinct lack of comments that help novices to understand the code. I feel that we almost have a privileged few mentality in some code; if you can't figure it out by reading the code, then you shouldn't be messing with it. Indeed. This sets a high barrier to entry when trying to learn about any nontrivial subsystem. Even when functions are kept brief and well-named, local simplicity does not eliminate global complexity; in fact it can mask it, making the system appear saner than it really is. In this sense, I disagree that self-documenting code is possible on a large scale (even though I am certainly a fan of making the small-scale units concise, clear, etc.). Back when we were writing the initial Chromium port, Brett Wilson and I had to rewrite the Image subsystem three separate times, each time because we realized we'd gotten the ownership semantics wrong and thought we now understood things better. In this case, a simple function that merely allocates an object is deceptively self-documenting, because it's clear what it does in a local sense, but not in the larger picture of how it fits into the rest of the codebase. No one is suggesting that silly comments like for (int i = 0; i 10; ++i) // Count to 10. are a good thing, but I have never had any reviewer objections to adding more detailed comments about subtle bits. At the very least I agree with Simon's suggestion of class-level comments, especially w.r.t. ownership. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Fri, Jan 28, 2011 at 10:14 AM, Alexey Proskuryakov a...@webkit.org wrote: Do you have any specific mechanism in mind for keeping global comments accurate? No more than I have for keeping API usage or function implementations correct; that is, if we have comments, they must be as important to reviewers and patch authors as the code is. Personally, I expect that adding such comments will make the barrier for entry higher in practice, as one would have to deal with lies. At least for me, WebKit subsystems have always been much harder to learn than equivalent-complexity Chromium subsystems (in Chromium we strongly encourage comments, and our style guide mandates at least class-level ones). I have heard this sentiment echoed by others but I can't say whether it's a majority viewpoint. No system is perfect, but I have not found out-of-date comments in Chromium to be a big problem, presumably because the culture prods people to keep them up to date. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Pixel tests and displaying text
If you never write layout tests, you can stop reading. Right now few ports run pixel tests (a shame). One reason is because we frequently need different reference images for each port, and creating hundreds or thousands of these is a hassle. Maintenance burden also increases. We could greatly decrease the number of these baselines by following a simple rule: don't display text unless you need to. For example, let's say I have a test that is supposed to display a green square. If the test output is: A green square means this test passes. If there is any red below, fail. [green square here] ...then for each platform that has different font rendering, whether that be subpixel AA differences, font size differences, or anything else, we'll need new baselines. By contrast, if that explanation was in an HTML comment inside the test code, and the output was a simple green square, one baseline would serve for all ports. I think this doesn't really hinder tracking down test failures, for a couple of reasons: * Most tests are green = pass, red = failure by convention, so frequently a pixel result that differs from the baseline is easily classifiable at a quick glance. * When this isn't the case, one of the first steps in understanding the test output is to read the test, at which point the HTML comment will explain things. Obviously, some tests need to display text, but this seems to me to be a good rule of thumb. Am I missing anything? If not, is anyone interested in helping to make this change on existing tests where possible, to trim the number of baselines in the tree and make it easier for new ports to bring up pixel tests? PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pixel tests and displaying text
On Wed, Dec 8, 2010 at 12:57 PM, Darin Adler da...@apple.com wrote: I’m worried a bit, though, that if we can’t use any text in them at all, the tests are then not at all self explanatory. You have to be an expert on the test to understand what it’s testing and what success and failure look like. I think for tests where we're worried that's the case, we should (for now) just go ahead and continue using text and having separate baselines. The per-test cost is relatively low and the number of tests that fall into this bucket is hopefully small? Maybe we can come up with a new form of test that puts the explanation of the test somewhere that will not be dumped and the test in a frame, and have the pixel tests dump only the frame. That would also work. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pixel tests and displaying text
On Wed, Dec 8, 2010 at 1:04 PM, Darin Adler da...@apple.com wrote: On Dec 8, 2010, at 1:02 PM, Peter Kasting wrote: the number of tests that fall into this bucket is hopefully small? Maybe we should talk about some specific tests. I can‘t think of many tests with no text that are self explanatory! It's not so much whether the test is self-explanatory as whether, given a result that differs from the baseline, it's obvious whether it's a failure and how to proceed. The particular change that triggered this thread, for example, was http://trac.webkit.org/changeset/73531 . The tests added in this commit are basically of the standard some number of green squares; red = failure form. Moving the explanatory text from them into the HTML for the tests themselves would not greatly decrease the ease with which someone could understand why they'd suddenly started failing the tests. Text is more useful when it's more likely that differences will occur that are not obviously either OK or wrong. In that case, having the text in the test output saves a trip to the HTML source on the part of the person trying to figure out whether he's seeing a failure. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pixel tests and displaying text
On Wed, Dec 8, 2010 at 2:03 PM, Darin Adler da...@apple.com wrote: You both have convinced me that HTML comments could be fine for explaining what a test is testing. Maybe we could come up with a format that makes it likely we’ll spot those comments. In the (few) tests I've written, I generally use top-of-file comments that describe both the expected results and what the underlying causes for potential failures are. See for example the layout test at the bottom of https://bugs.webkit.org/attachment.cgi?id=72387action=review . PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pixel tests and displaying text
On Wed, Dec 8, 2010 at 3:24 PM, Maciej Stachowiak m...@apple.com wrote: Maybe we could come up with a way to print the explanation in the browser only, and not in DumpRenderTree. A simple convention could be: if (!window.layoutTestController) document.write(Explanation of what the test is testing goes here.); This is slightly more convenient than an HTML comment when you open a test in the browser to see what's going on. At least when I've been tracking down test failures, the most convenient thing is the pixel output itself, which I have in both raw and diff form in the various Chromium dashboards. At the point when I need to manually open the test in some fashion, text displayed in the document versus text appearing in the source code are about equally accessible -- if anything, the latter is better for me (opening text editors is easy). Really, I suspect that all that really matters is that we establish some convention, so that I don't have to try opening the test outside the testing environment, reading the source, etc., all in hope of an explanation that I'm not sure exists. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
