Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Dec 2, 2009, at 9:57 PM, Dan Bernstein wrote: I am satisfied with the existing rule. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Wed, Dec 2, 2009 at 9:57 PM, Dan Bernstein m...@apple.com wrote: On Dec 2, 2009, at 9:46 PM, Peter Kasting wrote: On Wed, Dec 2, 2009 at 9:19 PM, Mark Rowe mr...@apple.com wrote: On 2009-12-02, at 21:00, Peter Kasting wrote: I find this tricky to read and error-prone. I propose that the rule be modified to be: * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. I do not agree that this would be an improvement. Are you satisfied with the existing rule, then? If so, you would be the first developer I have asked who is. I am satisfied with the existing rule. Me too. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Nice, that change would make it compatible with the Qt style guide! I:-) I
say go for it!
Kenneth
On Thu, Dec 3, 2009 at 2:00 AM, Peter Kasting pkast...@google.com wrote:
This is a followup to my thread yesterday regarding consistent enforcement
of the style guide. Like Yong Li, I find the current rule about braces on
conditional arms to be suboptimal. The current rule is that one-line arms
must not have braces. This leads to strange constructions like:
if (foo) {
a;
b;
c;
// etc., very long body
} else
x;
...or perhaps:
if (foo)
a;
else if (bar) {
b;
c;
} else if (baz)
d;
else if (qux) {
e;
f;
}
I find this tricky to read and error-prone. I propose that the rule be
modified to be:
* When all arms of a conditional or loop are one physical line, do not use
braces. If any arms are more than one physical line (even if they are one
logical line), use braces on all arms.
In most places this will not differ from the existing code, so it will not
cause the whole codebase to become invalid; but it prevents cases where
the inconsistency leads (IMO) to lower readability/safety. (As a bonus for
Chromium developers, it's compatible with the Google style guide too,
although it goes further than that guide in order to make the correct style
explicit in all cases.)
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
--
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] More on test flakiness
On Wed, 2009-12-02 at 14:51 -0800, Julie Parent wrote: As Eric just said to me in person, another option is to just re-run *any* failing test twice, and only turn tree red if it fails twice. (Chromium just recently started doing this, and it has greatly improved our tree greenness). This obviously doesn't explicitly identify timing dependent tests, but it solves the bigger issues that flaky tests cause. But that would turn moot the point of the suggestion, I think. Having only tests that are expected to fail under special conditions be tested twice makes sense, but if a test that isn't expected to fail under special conditions fails, we should see that as a failure. To give you a bit of insight into this, in GTK+ we used to have tests that only failed when they were preceded by a specific test. This was very important information that would be lost if it was run after itself, and thus passed. The problems that caused this were missing support in DRT, and a couple of times real bugs that caused crashes. This is to say I think we would be better served by only running known-time-dependent tests twice. Thanks, -- Gustavo Noronha Silva g...@gnome.org GNOME Project ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Platform API for Uniscribe/ComplexTextController?
On Dec 2, 2009, at 10:52 AM, Kevin Ollivier wrote: Hi Dan, On Dec 1, 2009, at 8:29 AM, Dan Bernstein wrote: On Nov 29, 2009, at 10:27 AM, Kevin Ollivier wrote: Hi all, The wx port is starting to look at getting proper support for complex text, and one of the first places I started looking at was the Win and Mac port implementations. I noticed that the complex text code in FontWin.cpp and FontComplexTextMac.cpp is largely the same, passing the heavy lifting down to their respective complex text controllers, and I actually wondered if they could be merged if there were some tweaks to the APIs to make them compatible. That led me to wonder if we couldn't make ComplexTextController one of our platform classes and have USE() defines to determine the implementation. Then we could move the drawComplexText, floatWidthForComplexText, and offsetForPositionForComplexText into Font.cpp guarded by that USE() define. The wx port can provide the native font handles the Win/Mac controllers need, so it'd really be great if we could just add these into our build to get complex text support working without having to implement the complex text methods differently for each platform. BTW, I actually already did get UniscribeController compiling, actually, but on Windows I had to have the build script copy it to platform/graphics/wx because MSVC implicitly puts the current directory first on the path, which was causing it to pick up platform/graphics/win/FontPlatformData.h instead of the wx one when compiling that file. :( So that's something else that would need to be addressed if ports can start sharing these files. Thanks, Kevin Hi Kevin, This sounds like a generally good idea. ComplextTextController is already using USE() macros to select between Core Text and ATSUI. I am not entirely sure how the the *ComplexText() methods will be guarded in Font.cpp in your proposal. Are you suggesting something like #if USE(ATSUI) || USE(CORE_TEXT) || USE(UNISCRIBE) || … ? I was thinking we'd create some WTF_USE_COMPLEX_TEXT_CONTROLLER so that it would be easier to opt out of using it, since a port may define / use WTF_USE_ATSUI or WTF_USE_CORE_TEXT for purposes other than supporting complex text. The USE() macros are intended for specifying platform technologies, not parts of the engine. I think it would be more appropriate to use ENABLE(COMPLEX_TEXT) to control the feature. There are still some differences in behavior between ComplexTextController and UniscribeController, so you’d need to find a way to accommodate them or eliminate them. In terms of public API, I think merging the changes shouldn't be too difficult. IIUC finalRoundingWidth() can probably just remain 0 on Win, and the ignore writing direction optimization on Mac can be put in the common API but just be ignored under Windows. The only thing I'm not sure about is that Mac and Win get the run's width in different ways, but I'm not quite sure why they do it differently00. Either approach would seem to work for both platforms. Mac calculates the total width when the ComplexTextController is created, while on Win to calculate it you have to call advance(run.length()) then get the value using runWidthSoFar(). Since the glyph used for the ith character—and the width of the glyph used for the ith character—may depend on the i+1th character, you have to generate glyphs and advances for the entire run before you can determine any widths, so ComplexTextController does it in its constructor. What UniscribeController leads to incorrect selection rects in some cases. The quick fix would seem to be to just have both platforms call advance(run.length()) and then use runWidthSoFar(), but I suspect that would hurt performance on Mac. Another way that might fix it would be to call advance(run.length()) in UniscribeController::UniscribeController then set m_totalWidth = m_runWidthSoFar and reset m_currentCharacter and m_runWidthSoFar to 0. Lastly, we could take the guts of advance and put it into something like a Win version of adjustGlyphsAndAdvances() that sets total width. Do you have any suggestions / preferences for how to tackle this? I would like UniscribeController to become more like ComplexTextController, not the other way around, but I wouldn’t like to make either one slower than. If the UniscribeController constructor advanced to the end, but then reset its current state, it would end up doing some amount of work twice in some cases, because the adjusted glyphs and advances it had produced in the first pass would be lost. Generally speaking, the way to fix this is to add “adjusted advances” and “adjusted glyphs” vectors to UniscribeController, like the ones ComplexTextController has. You’d probably also need a vector of runs (corresponding to Uniscribe “items”), but the differences between Uniscribe and the Mac complex text
[webkit-dev] improving String::append()
Hi, according to my measurements, a large chunk of WebKit memory consumption comes from WebCore::TextResourceDecoder::decode (loader/TextResourceDecoder.cpp:818) which calls String::append (String.cpp:82). The comment there says: // FIXME: This is extremely inefficient. I would volunteer to rewrite the code (especially reducing the memory consumption), but first, I would like to hear your opinion how can we make the string handling of WebKit better. Zoltan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] improving String::append()
I think we have a StringBuilder class that is optimized for building string incrementally. Other code uses VectorUChar and String::adopt. Maybe we should optimize the hot spots instead of changing Strings everywhere? Adam On Thu, Dec 3, 2009 at 8:32 AM, Zoltan Herczeg zherc...@inf.u-szeged.hu wrote: Hi, according to my measurements, a large chunk of WebKit memory consumption comes from WebCore::TextResourceDecoder::decode (loader/TextResourceDecoder.cpp:818) which calls String::append (String.cpp:82). The comment there says: // FIXME: This is extremely inefficient. I would volunteer to rewrite the code (especially reducing the memory consumption), but first, I would like to hear your opinion how can we make the string handling of WebKit better. Zoltan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] improving String::append()
I suggest using VectorUChar instead of String in TextResourceDecoder’s interface and implementation. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] improving String::append()
On Dec 3, 2009, at 8:32 AM, Zoltan Herczeg wrote: Hi, according to my measurements, a large chunk of WebKit memory consumption comes from WebCore::TextResourceDecoder::decode (loader/TextResourceDecoder.cpp:818) which calls String::append (String.cpp:82). The comment there says: // FIXME: This is extremely inefficient. I would volunteer to rewrite the code (especially reducing the memory consumption), but first, I would like to hear your opinion how can we make the string handling of WebKit better. I believe the inefficiency there is speed, not space. VectorUChar or StringBuilder as an alternative would reduce potential for excess reallocation but I don't think they would reduce memory use. In fact memory use may go up. (As a side note I was unable to find the String::append call in TextResourceDecoder. I think a lot of memory use is blamed on the decoder because we keep around a decoded copy of every text-based subresource. In theory we could drop the decoded version under memory pressure since it can be re-decoded from the original data on demand. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Wed, Dec 2, 2009 at 11:35 PM, Maciej Stachowiak m...@apple.com wrote:
I used to dislike everything about this rule; my original preference before
we codified our style guidelines was to always put braces around the body of
a control statement, even if it is only one line. Over time, I've gotten
used to it and I find this:
if (condA)
doA();
more pleasant to read than this:
if (condA) {
doA();
}
Even for if/else statements where only one branch is single-line, I'm ok
with the official style rule, although I can't say I actively like it. So
these two examples look ok to me:
if (condA)
doA();
else {
doB1();
doB2();
}
if (condA) {
doA1();
doA2();
} else
doB();
However, when there is a lengthy chain of if ... else if ... else
conditionals and a few of the blocks in the middle happens to be only a
single line each, then I tend to find the code harder to write, read and
modify. This pattern often comes up in the parseMappedAttribute()
implementations of Element subclasses.
Regardless of the specific outcome, I would strongly prefer to have a
consistent rule for this than to make it a matter of taste. One possible
conservative modification to the rule is that if you have a multi-block if
... else if ... else chain, then if even one body has braces, all should. I
think that would tend to reduce rather than increase visual noise, as all
the else keywords would line up where right now they look ragged.
I would also not object to bigger changes in the rule if that were truly the
community consensus, but personally I would set the barrier pretty high for
a rule change that would implicate large chunks of existing code, and I
think the benefit is much lower for if/else statements with only two
clauses.
In the WebKit WebGL implementation I've frequently encountered the
case where the else clause in a single if/else is a one-liner, and I
find it both ugly and error-prone to have to remove its braces. I'd
really like to be able to use braces on both arms.
Perhaps existing code could be grandfathered in but new or modified
code required to conform to the rule Peter proposes.
-Ken
Regards,
Maciej
On Dec 2, 2009, at 9:00 PM, Peter Kasting wrote:
This is a followup to my thread yesterday regarding consistent enforcement
of the style guide. Like Yong Li, I find the current rule about braces on
conditional arms to be suboptimal. The current rule is that one-line arms
must not have braces. This leads to strange constructions like:
if (foo) {
a;
b;
c;
// etc., very long body
} else
x;
...or perhaps:
if (foo)
a;
else if (bar) {
b;
c;
} else if (baz)
d;
else if (qux) {
e;
f;
}
I find this tricky to read and error-prone. I propose that the rule be
modified to be:
* When all arms of a conditional or loop are one physical line, do not use
braces. If any arms are more than one physical line (even if they are one
logical line), use braces on all arms.
In most places this will not differ from the existing code, so it will not
cause the whole codebase to become invalid; but it prevents cases where
the inconsistency leads (IMO) to lower readability/safety. (As a bonus for
Chromium developers, it's compatible with the Google style guide too,
although it goes further than that guide in order to make the correct style
explicit in all cases.)
PK
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 11:20 AM, Alexey Proskuryakov a...@webkit.org wrote: On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. I would like this. On Thu, Dec 3, 2009 at 11:30 AM, Kenneth Russell k...@google.com wrote: Perhaps existing code could be grandfathered in but new or modified code required to conform to the rule Peter proposes. This is more or less the norm. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 11:36 AM, Jeremy Orlow jor...@chromium.org wrote: On Thu, Dec 3, 2009 at 11:20 AM, Alexey Proskuryakov a...@webkit.org wrote: On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. I would like this. While I personally believe every branch should be wrapped in braces, it seems like we're not going to get consensus on that. I can live with no braces on any branch, but mixing brace-wrapped branches and non-wrapped is really distasteful. So, I would vote for this suggestion, as it gets us closer to what I believe is good. -- Dirk On Thu, Dec 3, 2009 at 11:30 AM, Kenneth Russell k...@google.com wrote: Perhaps existing code could be grandfathered in but new or modified code required to conform to the rule Peter proposes. This is more or less the norm. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 11:36 AM, Jeremy Orlow jor...@chromium.org wrote: On Thu, Dec 3, 2009 at 11:20 AM, Alexey Proskuryakov a...@webkit.orgwrote: On 02.12.2009, at 23:35, Maciej Stachowiak wrote: One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. That would be my preference, too. I would like this. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Not Implemented selectAll
On Dec 2, 2009, at 11:28 PM, Brent Fulgham wrote: I notice that the IWebFrame interface provides a stub for this through the IWebDocumentText interface, which I was happily connecting to when I realized that it dead-ends in a notImplemented call. Is this just something no one has completed implementing? Yes. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Switch statement indentation
Sounds good to me. Geoff On Dec 2, 2009, at 8:58 PM, Peter Kasting wrote: On Wed, Dec 2, 2009 at 8:05 PM, Maciej Stachowiak m...@apple.com wrote: I believe one rule that could work is something like this: - Indent case labels inside a switch two spaces. - Indent actual statements inside a switch four spaces. - In the case where a case label is followed by a block, include the open brace on the same line as the case label and indent the matching close brace only two spaces (but still 4 spaces for the contained statements). This is precisely the rule that the Google C++ Style Guide uses ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Switch_Statements ). I think it works well. I support it. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Sharing code between WebCore/bindings/js and WebCore/bindings/v8
Sorry for the thread necromancy, but this has come back to the top of my list. On Thu, May 28, 2009 at 2:07 AM, Maciej Stachowiak m...@apple.com wrote: On May 27, 2009, at 11:47 AM, Adam Barth wrote: I've been doing some work recently in our JavaScript bindings. As part of this work, I've noticed that WebCore/bindings/js and WebCore/bindings/v8 have drifted apart in some details. It's kind of ridiculous that we have so much duplicated code in these two folders. We should try to re-organize our bindings to share as much code as possible. [...] It's definitely unfortunate that the bindings keep going out of sync. I have two concerns though: 1) We weren't super enthusiastic about the master WebKit tree trying to support two different JavaScript engines. But we finally agreed when the Chrome folks said this was a hard requirement to merge, and promised they would take on absolutely 100% of the maintenance burden and impose no cost on the rest of the WebKit project. As a result: 1.a) We're not super keen on refactorings that wouldn't be justified just for JSC itself. If there is shareable code that is easily factored out without adding otherwise needless abstraction or layers of indirection, that's fine, but we don't want to add indirection that would add any code complexity or runtime cost. 1.b) We couldn't commit to not breaking the v8 bindings when hacking on the shared layer, if there was one. We're going to address these concerns by doing the V8 bindings first and not touching the JSC bindings at all. Basically, the plan is to move common code out of the V8 bindings and abstract away the V8-specific bits with an eye towards making the code useful for JSC. We're going to start small and see how it goes. Once we've done enough of that to make the direction clear, we can decide whether JSC wants to use the common bits. 2) My understanding is that the v8 bindings are not even fully merged into the WebKit tree yet - many pieces live externally in the Chromium repository. So we couldn't meaningfully maintain the v8 bindings even if we wanted to, and would have no good way to evaluate the soundness of the shared layer. This issue is now resolved because the V8 bindings are fully upstream. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
Should the namespace-indent rule apply even to nested namespaces? I
understand the reasoning behind the rule — it's a waste of horizontal
space to indent almost everything in the file — but if a secondary
namespace is declared, it would seem weird not to indent its lines
either.
This comes up because I have a patch out for review that includes the
addition of an HTTPHeaders namespace that just contains a bunch of
string constants:
// HTTPHeaderMap.h
namespace WebCore {
class HTTPHeaderMap : public HashMapAtomicString, String,
CaseFoldingHash {
...
...
};
namespace HTTPHeaders {
extern const char* const Accept;
extern const char* const Authorization;
...
extern const char* const UserAgent;
}
}
This would look strange if the contents of HTTPHeaders weren't
indented. Especially since if HTTPHeaders were made a class instead
(which I almost decided to do) the style guide _would_ say to indent it.
namespace HTTPHeaders {
extern const char* const Accept;
extern const char* const Authorization;
...
extern const char* const UserAgent;
}
—Jens
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
On 03.12.2009, at 14:22, Jens Alfke wrote: This comes up because I have a patch out for review that includes the addition of an HTTPHeaders namespace that just contains a bunch of string constants: I do not think constants for HTTP headers are part of HTTPHeaderMap - maybe it would be better to add them to a new file. That would resolve the issue with style automatically, although I agree with the rationale you provided. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
On Dec 3, 2009, at 3:40 PM, David Levin wrote: So far it seems at least two people agree with this and no one objected last time. The next appropriate step if you want the issue fixed is to file a bug on the style guide and update it. https://bugs.webkit.org/show_bug.cgi?id=32137 Working on a patch now. Is there a meta-style guide for updates to the style guide? ;) —Jens ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] namespace indent
btw, that you put it this way :) This issue came up before: First, it seems like the original motive was to avoid pointlessly indenting nearly the whole file: https://lists.webkit.org/pipermail/webkit-dev/2009-September/010002.html So, I was wondering if we can clarify the rule to apply only to the outermost namespace declaration. So, I was wondering if we can clarify the rule to apply only to the outermost namespace declaration. [Darin Adler's reply] Yes, I think we can. So far it seems at least two people agree with this and no one objected last time. The next appropriate step if you want the issue fixed is to file a bug on the style guide and update it. dave On Thu, Dec 3, 2009 at 3:33 PM, Alexey Proskuryakov a...@webkit.org wrote: On 03.12.2009, at 14:22, Jens Alfke wrote: This comes up because I have a patch out for review that includes the addition of an HTTPHeaders namespace that just contains a bunch of string constants: I do not think constants for HTTP headers are part of HTTPHeaderMap - maybe it would be better to add them to a new file. That would resolve the issue with style automatically, although I agree with the rationale you provided. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Why Page::setMainFrame use PassRefPtr type parameter?
On Nov 28, 2009, at 12:09 AM, pattin.shieh wrote: Look at this:void Page::setMainFrame(PassRefPtrFrame mainFrame). Why use PassRefPtr? Because the only call to Page::setMainFrame is inside the Frame constructor, there is no performance benefit to taking a PassRefPtr. But it’s arguably a good practice for a function that takes ownership of its argument to take a PassRefPtr even in cases where there is no performance benefit. It’s how the function states that it will keep a reference. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
While I personally believe every branch should be wrapped in braces, it seems like we're not going to get consensus on that. I can live with no braces on any branch, but mixing brace-wrapped branches and non-wrapped is really distasteful. So, I would vote for this suggestion, as it gets us closer to what I believe is good. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Strings on multiple threads
Handling strings on multiple threads is driving me crazy. There are so many subtleties about what's safe and what's not I'm wondering if it makes sense to just make a thread safe class. For example, String1 + String2 is not safe if either of those threads is from another thread (since if either string is empty, it'll just point to the other strings StringImpl). Another example: if you have a class that isn't always destroyed on the same thread and anything is making a copy of a string it owns and that copy might outlive the class, the copy has to be thread safe. I understand that making all strings ThreadSafeShared is completely out of the question, but maybe we can make another class of strings that is safe? Maybe we can even do it in a way that still shares the majority of the existing string code? (Ideally without resorting to templates.) I'm asking here before looking at this seriously since I'm guessing this has come up before and/or there are good reasons why this hasn't been done before. Thanks, J ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Strings on multiple threads
Just to be clear, I'm wondering if it makes sense to create a threadsafe string class for use in places that aren't performance intensiveand definitely not a replacement to the current string class. On Thu, Dec 3, 2009 at 5:21 PM, Jeremy Orlow jor...@chromium.org wrote: Handling strings on multiple threads is driving me crazy. There are so many subtleties about what's safe and what's not I'm wondering if it makes sense to just make a thread safe class. For example, String1 + String2 is not safe if either of those threads is from another thread (since if either string is empty, it'll just point to the other strings StringImpl). Another example: if you have a class that isn't always destroyed on the same thread and anything is making a copy of a string it owns and that copy might outlive the class, the copy has to be thread safe. I understand that making all strings ThreadSafeShared is completely out of the question, but maybe we can make another class of strings that is safe? Maybe we can even do it in a way that still shares the majority of the existing string code? (Ideally without resorting to templates.) I'm asking here before looking at this seriously since I'm guessing this has come up before and/or there are good reasons why this hasn't been done before. Thanks, J ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Strings on multiple threads
On Thu, Dec 3, 2009 at 5:21 PM, Jeremy Orlow jor...@chromium.org wrote: Handling strings on multiple threads is driving me crazy. My recommendation don't use strings on multiple threads. If you do, it has a high chance of being wrong because it is very subtle so try not to do it. Instead creating a string for another thread is using crossThreadString which is relatively cheap (sorry about the name, I have a bug on fixing that just haven't had time to think of something better). (Note that method isn't threadsafe.) For example, String1 + String2 is not safe if either of those threads is from another thread (since if either string is empty, it'll just point to the other strings StringImpl). See above : don't do it :) Another example: if you have a class that isn't always destroyed on the same thread and anything is making a copy of a string it owns Don't hand out strings from such a class. Only hand out strings that come from the crossThreadString method or the copy method. Or only give out a UChar*. (Of course, it would be nice if you could make your class always be destroyed on the same thread or at least destroy all of its strings on the same thread where they were used.) I understand that making all strings ThreadSafeShared is completely out of the question, but maybe we can make another class of strings that is safe? Maybe we can even do it in a way that still shares the majority of the existing string code? (Ideally without resorting to templates.) Or is there a way to change your code to not use the same string class on multiple threads? A big issue is the RefCounted base for StringImpl. (Next you'd also have problems with append, insert, etc.) dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
what peter pitched and what mjs pitched are one and the same i think * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. +1 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Thu, Dec 3, 2009 at 5:48 PM, Michael Nordman micha...@google.com wrote: what peter pitched and what mjs pitched are one and the same i think No, they are not. Both of us would transform a conditional with three arms. Only I would transform one with two arms. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On 2009-12-03, at 17:48, Michael Nordman wrote: what peter pitched and what mjs pitched are one and the same i think They are different. Maciej's proposal was more conservative in that it only applies when multiple else blocks are present. * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. +1 / πr2 - Mark smime.p7s Description: S/MIME cryptographic signature ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Strings on multiple threads
On Thu, Dec 3, 2009 at 5:46 PM, David Levin le...@chromium.org wrote: On Thu, Dec 3, 2009 at 5:21 PM, Jeremy Orlow jor...@chromium.org wrote: Handling strings on multiple threads is driving me crazy. My recommendation don't use strings on multiple threads. If you do, it has a high chance of being wrong because it is very subtle so try not to do it. Instead creating a string for another thread is using crossThreadString which is relatively cheap (sorry about the name, I have a bug on fixing that just haven't had time to think of something better). (Note that method isn't threadsafe.) For example, String1 + String2 is not safe if either of those threads is from another thread (since if either string is empty, it'll just point to the other strings StringImpl). See above : don't do it :) Another example: if you have a class that isn't always destroyed on the same thread and anything is making a copy of a string it owns Don't hand out strings from such a class. Only hand out strings that come from the crossThreadString method or the copy method. Or only give out a UChar*. (Of course, it would be nice if you could make your class always be destroyed on the same thread or at least destroy all of its strings on the same thread where they were used.) Easier said than done in many cases. For DOM Storage and Databases, there are several classes that are used on multiple threads. And several of these classes contain strings (or contain things that contain strings). It seems very difficult to be absolutely sure these strings are only shared in a safe way unless you make a threadsafeCopy every time you access it. Doing so would probably be negligible performance wise for all the cases I'm looking at, so maybe that's the right answer, but these issues are very subtle. And that's what concerns me most. I understand that making all strings ThreadSafeShared is completely out of the question, but maybe we can make another class of strings that is safe? Maybe we can even do it in a way that still shares the majority of the existing string code? (Ideally without resorting to templates.) Or is there a way to change your code to not use the same string class on multiple threads? A big issue is the RefCounted base for StringImpl. (Next you'd also have problems with append, insert, etc.) Exactlyso we couldn't use StringImpl as is. We could, however, wrap all access to methods that could ref it and/or mutate it with a lock (as an example). I just don't want to have to keep thinking about the subtle threading issues of strings in cases where there's no way it could be performance critical. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
On Dec 3, 2009, at 5:48 PM, Michael Nordman wrote: what peter pitched and what mjs pitched are one and the same i think * When all arms of a conditional or loop are one physical line, do not use braces. If any arms are more than one physical line (even if they are one logical line), use braces on all arms. One possible conservative modification to the rule is that if you have a multi-block if ... else if ... else chain, then if even one body has braces, all should. I think that would tend to reduce rather than increase visual noise, as all the else keywords would line up where right now they look ragged. To be clear: my intent was to propose something different than Peter's suggestion. Specifically, I suggest we do *not* change the rule for a simple if/else statement with only two clauses. Those would still be able to have braces on only one of the two clauses. But if/else chains of more than two clauses would have to be consistent. For anyone who agreed with my suggestion, you may want to consider whether you still agree in light of this clarification. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Date: Wed, 2 Dec 2009 21:00:59 -0800 From: Peter Kasting pkast...@google.com This is a followup to my thread yesterday regarding consistent enforcement of the style guide. Like Yong Li, I find the current rule about braces on conditional arms to be suboptimal. For the people thinking about this, it would be nice if the final policy minimizes (and does not increase) the likelihood of having to modify several lines of surrounding code after touching one line of code. I don't think this consideration has been raised. This is similar to a point Darin Adler raised a few months back with regard to lining up comments: Things manually lined up in source code generally create a small maintainability problem. You can’t rename things without re-lining things up, and if you make other types code changes without noticing the lined-up comments the code ends up looking messy an peculiar. ( https://lists.webkit.org/pipermail/webkit-dev/2009-September/009814.html ) --Chris ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] SharedScript: next steps and result of offline discussion.
Hi WebKit, The recent discussion indicated there is a feeling that SharedScript functionality can be achieved by other means that do not require adding new big APIs: changing iframe a bit (so it's internals survive reparenting into another document) and adding single getWindowByName() API. Ian Hickson proposed this idea noting that nothing in the spec prevents iframe from staying alive over the reparenting. Some folks from Google met with folks from Apple to discuss and it appears there is a consensus that we shall remove initial code for SharedScript and instead implement changes for iframe and getWindowByFrame(). This will not cause new API and hopefully won't cause compatibility issues since the only scenario that will behave differently is reparenting of the iframe between documents that is hopefully a rare thing. Perhaps more discussion will be needed to nail details on those. Separately (or related?), we discussed SharedWorkers and the way XHR works in them. It seems a good idea to investigate a shadow document' solution (as Chrome does for worker processes) when a dummy document is created and used to load resources on behalf of the worker. Also we'll try to fix couple of bugs that prevent Workers to be reliable enough. Thanks a lot to all who chimed in and helped to arrive to a good solution to the same issues that SharedScript was trying to solve! :-) Dmitry Titov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
I'm OK with both. Peter's is my preferred solution, though.
Kenneth
On Thu, Dec 3, 2009 at 11:39 PM, Dirk Pranke dpra...@chromium.org wrote:
+1. Peter's is better than Maciej's is better than status quo.
-- Dirk
On Thu, Dec 3, 2009 at 6:24 PM, Jeremy Orlow jor...@chromium.org wrote:
On Thu, Dec 3, 2009 at 6:15 PM, Maciej Stachowiak m...@apple.com wrote:
On Dec 3, 2009, at 5:48 PM, Michael Nordman wrote:
what peter pitched and what mjs pitched are one and the same i think
* When all arms of a conditional or loop are one physical line, do not
use
braces. If any arms are more than one physical line (even if they are
one
logical line), use braces on all arms.
One possible conservative modification to the rule is that if you
have
a multi-block if ... else if ... else chain, then if even one body
has
braces, all should. I think that would tend to reduce rather than
increase
visual noise, as all the else keywords would line up where right
now they
look ragged.
To be clear: my intent was to propose something different than Peter's
suggestion. Specifically, I suggest we do *not* change the rule for a
simple
if/else statement with only two clauses. Those would still be able to
have
braces on only one of the two clauses. But if/else chains of more than
two
clauses would have to be consistent.
For anyone who agreed with my suggestion, you may want to consider
whether
you still agree in light of this clarification.
Is this formally up for a vote then?
For what it's worth, I think Peters is the most readable, but Maciej's is
better than what it is now. I find stuff like
if (blah)
blah
else {
blah
}
very distracting. But stuff like
if (blah)
blah
else if (blah)
blah
else if (blah) {
blah
} else
blah
is definitely even worse.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
--
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Proposing style guide change regarding braces on conditional arms
Ditto.
On Thu, Dec 3, 2009 at 7:25 PM, Kenneth Christiansen
kenneth.christian...@openbossa.org wrote:
I'm OK with both. Peter's is my preferred solution, though.
Kenneth
On Thu, Dec 3, 2009 at 11:39 PM, Dirk Pranke dpra...@chromium.org wrote:
+1. Peter's is better than Maciej's is better than status quo.
-- Dirk
On Thu, Dec 3, 2009 at 6:24 PM, Jeremy Orlow jor...@chromium.org wrote:
On Thu, Dec 3, 2009 at 6:15 PM, Maciej Stachowiak m...@apple.com
wrote:
On Dec 3, 2009, at 5:48 PM, Michael Nordman wrote:
what peter pitched and what mjs pitched are one and the same i think
* When all arms of a conditional or loop are one physical line, do not
use
braces. If any arms are more than one physical line (even if they are
one
logical line), use braces on all arms.
One possible conservative modification to the rule is that if you
have
a multi-block if ... else if ... else chain, then if even one body
has
braces, all should. I think that would tend to reduce rather than
increase
visual noise, as all the else keywords would line up where right
now they
look ragged.
To be clear: my intent was to propose something different than Peter's
suggestion. Specifically, I suggest we do *not* change the rule for a
simple
if/else statement with only two clauses. Those would still be able to
have
braces on only one of the two clauses. But if/else chains of more than
two
clauses would have to be consistent.
For anyone who agreed with my suggestion, you may want to consider
whether
you still agree in light of this clarification.
Is this formally up for a vote then?
For what it's worth, I think Peters is the most readable, but Maciej's
is
better than what it is now. I find stuff like
if (blah)
blah
else {
blah
}
very distracting. But stuff like
if (blah)
blah
else if (blah)
blah
else if (blah) {
blah
} else
blah
is definitely even worse.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
--
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
