As the previous bug contains too much patches,
( https://bugs.webkit.org/show_bug.cgi?id=26620 )
I fill a new bug with only patches related to Haiku-specific
files contained in WebCore. ( WebCore/platform/haiku )
Comment on attachment 32300[details]
Patch to add a first bunch of Haiku-specific files for WebCore.
> +// extensions beyond IE's API
> +HashSet<String> ClipboardHaiku::types() const
> +{
> + HashSet<String> result;
> + BMessage *data;
> +
> + if (be_clipboard->Lock()) {
> + if ((data = be_clipboard->Data())) {
> + char *name;
> + uint32 type;
> + int32 count;
> +
> + for (int32 i = 0;
> + data->GetInfo(B_ANY_TYPE, i, &name, &type, &count) == B_OK;
> + i++) {
> + result.add(name);
> + }
This should be two lines:
for (int32 i = 0; data->GetInfo(B_ANY_TYPE, i, &name, &type, &count) == B_OK; i++)
result.add(name);
> +bool ClipboardHaiku::hasData()
> +{
> + bool result = false;
> + BMessage *data;
> +
> + if (be_clipboard->Lock()) {
> + if ((data = be_clipboard->Data())) {
> + result = !data->IsEmpty();
> + }
This if-statement has a single line of code so should not have braces
Rest of the patch looks fine, but it's easier for us to do landing if we have a set of correct patches to land
Comment on attachment 32303[details]
Patch to add a fourth bunch of Haiku-specific files for WebCore.
> +void Pasteboard::writeSelection(Range* selectedRange, bool canSmartCopyOrDelete, Frame* frame)
> +{
> + BMessage* data;
> +
> + if (be_clipboard->Lock()) {
> + be_clipboard->Clear();
> + if ((data = be_clipboard->Data())) {
> + data->AddString("text/plain", BString(frame->selectedText()));
> + be_clipboard->Commit();
> + }
> + be_clipboard->Unlock();
> + }
> +}
You'll want a bug to track supporting pasting of rich text. Also i'm concerned about where be_clipboard comes from? If it's the global pasteboard in haiku you don't want to use it here
> +String Pasteboard::plainText(Frame* frame)
> +{
> + BString string;
> + BMessage* data;
> +
> + if (be_clipboard->Lock()) {
> + if ((data = be_clipboard->Data())) {
> + data->FindString("text/plain", &string);
> + }
No braces on a single line if statement
(In reply to comment #11)
> (From update of attachment 32303[details])
> > +void Pasteboard::writeSelection(Range* selectedRange, bool canSmartCopyOrDelete, Frame* frame)
> > +{
> > + BMessage* data;
> > +
> > + if (be_clipboard->Lock()) {
> > + be_clipboard->Clear();
> > + if ((data = be_clipboard->Data())) {
> > + data->AddString("text/plain", BString(frame->selectedText()));
> > + be_clipboard->Commit();
> > + }
> > + be_clipboard->Unlock();
> > + }
> > +}
>
> You'll want a bug to track supporting pasting of rich text. Also i'm concerned
> about where be_clipboard comes from? If it's the global pasteboard in haiku
> you don't want to use it here
Indeed be_clipboard is system pasteboard in Haiku.
So I used a clipboard which should be specific to WebKit.
> > You'll want a bug to track supporting pasting of rich text. Also i'm concerned
> > about where be_clipboard comes from? If it's the global pasteboard in haiku
> > you don't want to use it here
>
> Indeed be_clipboard is system pasteboard in Haiku.
> So I used a clipboard which should be specific to WebKit.
Sorry i'll re-look at this, i keep forgetting whether pasteboard or clipboard is the generic system clipboard and which is a general interface to either dragging or copy/paste
(In reply to comment #23)
> > > You'll want a bug to track supporting pasting of rich text. Also i'm concerned
> > > about where be_clipboard comes from? If it's the global pasteboard in haiku
> > > you don't want to use it here
> >
> > Indeed be_clipboard is system pasteboard in Haiku.
> > So I used a clipboard which should be specific to WebKit.
>
> Sorry i'll re-look at this, i keep forgetting whether pasteboard or clipboard
> is the generic system clipboard and which is a general interface to either
> dragging or copy/paste
Your review was good.
As I looked to other ports, for the Pasteboard class they use a "per application" clipboard;
and in the Clipboard class a generic system clipboard.
Comment on attachment 32301[details]
Patch to add a second bunch of Haiku-specific files for WebCore.
Committing to http://svn.webkit.org/repository/webkit/trunk ...
M WebCore/ChangeLog
A WebCore/platform/haiku/ContextMenuHaiku.cpp
A WebCore/platform/haiku/ContextMenuItemHaiku.cpp
A WebCore/platform/haiku/DragDataHaiku.cpp
A WebCore/platform/haiku/DragImageHaiku.cpp
Committed r46019
M WebCore/ChangeLog
A WebCore/platform/haiku/DragImageHaiku.cpp
A WebCore/platform/haiku/DragDataHaiku.cpp
A WebCore/platform/haiku/ContextMenuItemHaiku.cpp
A WebCore/platform/haiku/ContextMenuHaiku.cpp
r46019 = 8ccf0e8fc838731509b10bb96dba66ed5b5148ec (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46019
Comment on attachment 32302[details]
Patch to add a third bunch of Haiku-specific files for WebCore.
Committing to http://svn.webkit.org/repository/webkit/trunk ...
M WebCore/ChangeLog
A WebCore/platform/haiku/EventLoopHaiku.cpp
A WebCore/platform/haiku/FileChooserHaiku.cpp
A WebCore/platform/haiku/FileSystemHaiku.cpp
A WebCore/platform/haiku/KeyboardCodes.h
A WebCore/platform/haiku/MIMETypeRegistryHaiku.cpp
Committed r46020
M WebCore/ChangeLog
A WebCore/platform/haiku/KeyboardCodes.h
A WebCore/platform/haiku/FileChooserHaiku.cpp
A WebCore/platform/haiku/MIMETypeRegistryHaiku.cpp
A WebCore/platform/haiku/FileSystemHaiku.cpp
A WebCore/platform/haiku/EventLoopHaiku.cpp
r46020 = d619f1bbc62f49f146cd04db11e35b56a8b6de97 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46020
Created attachment 32940[details]
Patch to add ScrollBar and ScrollView code specific to Haiku.
Many coding style issues corrected from the previous patch. ( Thanks to the cpplint.py tool :)
Created attachment 32943[details]
Patch to add ScrollBar and ScrollView code specific to Haiku.
I should build my code once before submitting patches.
I changed the name of an attribut, but I forgot two places in my source code.
Comment on attachment 32868[details]
Patch to add a first bunch of Haiku-specific files for WebCore.
> diff --git a/WebCore/platform/haiku/ClipboardHaiku.cpp b/WebCore/platform/haiku/ClipboardHaiku.cpp> +#include "HashTable.h"
Should be "#include <wtf/HashTable.h>"
> +
> +
> +namespace WebCore {
> +
> +ClipboardHaiku::ClipboardHaiku(ClipboardAccessPolicy policy, bool forDragging)
> + : Clipboard(policy, forDragging)
> +{
> +}
> +
> +void ClipboardHaiku::clearData(const String& type)
> +{
> + BMessage *data = (BMessage *) NULL;
1. Star in wrong place.
BMessage* data
2. Use c++ style casts. (BMessage *) (but I don't think the cast is necessary.
3. Use 0 instead of NULL.
This line is repeated a lot in this patch, but I'm only flagging this first instace.
> +
> + if (be_clipboard->Lock()) {
> + if ((data = be_clipboard->Data())) {
Since there doesn't seem to be any benefit of putting this in the if other than saving a line. Why not write it as two lines (which in my opinion makes it more readable)?
data = be_clipboard->Data();
if (data) {
It isn't necessary to be this but it would be nice imo.
> +// extensions beyond IE's API
Format comments like sentences (begin with a capital and end with a period).
> +IntPoint ClipboardHaiku::dragLocation() const
> +{
> + notImplemented();
> + return IntPoint(0,0);
Add a space after the comma.
> +DragImageRef ClipboardHaiku::createDragImage(IntPoint& dragLoc) const
Avoid abbreviations: dragloc -> dragLocation
> diff --git a/WebCore/platform/haiku/ClipboardHaiku.h b/WebCore/platform/haiku/ClipboardHaiku.h
> + // State available during IE's events for drag and drop and copy/paste
> + class ClipboardHaiku : public Clipboard {
> + public:
> + ClipboardHaiku(ClipboardAccessPolicy policy, bool forDragging);
"bool forDragging"
Please use an enum instead of a bool. (See http://trac.webkit.org/browser/trunk/WebCore/workers/WorkerScriptLoader.h for an example of where this was done.)
> diff --git a/WebCore/platform/haiku/CursorHaiku.cpp b/WebCore/platform/haiku/CursorHaiku.cpp> +namespace WebCore {
> +
> +Cursor::Cursor(PlatformCursor p)
"p" -- Use words instead f abbreviations for variable names.
> +static Cursor global_cursor((PlatformCursor)B_CURSOR_SYSTEM_DEFAULT);
Improper casing global_cursor -> globalCursor
Use C++ style casting (static_cast/reinterpret_cast, etc.) instead of C style casting.
> +static Cursor ibeam_cursor((PlatformCursor)B_CURSOR_I_BEAM);
Same comments as above.
(In reply to comment #31)
> > +ClipboardHaiku::ClipboardHaiku(ClipboardAccessPolicy policy, bool forDragging)
> > + : Clipboard(policy, forDragging)
> > +{
> > +}
> > +
> > +void ClipboardHaiku::clearData(const String& type)
> > +{
> > + BMessage *data = (BMessage *) NULL;
> 1. Star in wrong place.
> BMessage* data
>
> 2. Use c++ style casts. (BMessage *) (but I don't think the cast is necessary.
>
> 3. Use 0 instead of NULL.
Strange, I thought I corrected these style problems.
Anyway, it's done in the new patch. :)
> > diff --git a/WebCore/platform/haiku/ClipboardHaiku.h b/WebCore/platform/haiku/ClipboardHaiku.h
> > + // State available during IE's events for drag and drop and copy/paste
> > + class ClipboardHaiku : public Clipboard {
> > + public:
> > + ClipboardHaiku(ClipboardAccessPolicy policy, bool forDragging);
>
> "bool forDragging"
> Please use an enum instead of a bool. (See
> http://trac.webkit.org/browser/trunk/WebCore/workers/WorkerScriptLoader.h for
> an example of where this was done.)
On this point I don't really agree. I know what you mean but, as I looked in other ports,
none of them used an enum but a boolean.
However, I corrected something in ClipboardHaiku.h: other ports designed this class
with the constructor as private and have a static method to create the clipboard. So did I.
Comment on attachment 33093[details]
Patch to add a first bunch of Haiku-specific files for WebCore.
Just a few last things to address.
> Other ports designed this class with the constructor as private and have a static method to create the
> clipboard.
Yep, that is a standard pattern. My mistake in missing it :)
> > "bool forDragging"
> > Please use an enum instead of a bool. (See
> > http://trac.webkit.org/browser/trunk/WebCore/workers/WorkerScriptLoader.h for
> > an example of where this was done.)> On this point I don't really agree. I know what you mean but, as I looked in
> other ports, none of them used an enum but a boolean.
This is a new style that WebKit is moving to that likely post dates the other implementations, so that's why they use an enum.
For example see https://bugs.webkit.org/show_bug.cgi?id=26695, where this was said when someone was adding a new bool parameter:
We're trying to avoid adding boolean parameters, because they are not very
clear (when reading code like "hitTestResultAtPoint(point, true, false, true)",
what do the booleans mean?). One strategy to replace a boolean parameter is to
use a two-value enum, like this:
enum HitTestScrollbars { ShouldHitTestScrollbars, DontHitTestScrollbars };
Although in this case, it does just pass it along to the base class which takes bool, so perhaps it really isn't a new bool parameter.
Thus, it seems ok in this instance. Please keep this in mind when doing other apis though.
> diff --git a/WebCore/platform/haiku/ClipboardHaiku.cpp b/WebCore/platform/haiku/ClipboardHaiku.cpp> +#include "IntPoint.h"
> +#include "FileList.h"
> +#include "PlatformString.h"
> +#include "StringHash.h"
> +
> +#include "NotImplemented.h"
Please sort this with the other "" headers.
> +
> +#include <wtf/HashTable.h>
Please sort this with the other #include <> headers.
> +
> +#include <app/Clipboard.h>
> +#include <Message.h>
> +#include <String.h>> +void ClipboardHaiku::clearData(const String& type)
> +{
> + BMessage* data;
It would be better if you declared this where it was used and initialized.
> + if (be_clipboard->Lock()) {
> + data = be_clipboard->Data();
It would look like this:
BMessage* data = be_clipboard->Data();
This same pattern is throughout this code.
> diff --git a/WebCore/platform/haiku/ClipboardHaiku.h b/WebCore/platform/haiku/ClipboardHaiku.h> +
> +#include <support/Locker.h>
> +#include <app/Clipboard.h>
Please sort these two.
> diff --git a/WebCore/platform/haiku/CursorHaiku.cpp b/WebCore/platform/haiku/CursorHaiku.cpp
> +static Cursor global_cursor(static_cast<PlatformCursor>(B_CURSOR_SYSTEM_DEFAULT));
> +static Cursor ibeam_cursor(static_cast<PlatformCursor>(B_CURSOR_I_BEAM));
As noted before: Improper casing global_cursor -> globalCursor
(In reply to comment #34)
>
> This is a new style that WebKit is moving to that likely post dates the other
> implementations, so that's why they use an enum.
>
> For example see https://bugs.webkit.org/show_bug.cgi?id=26695, where this was
> said when someone was adding a new bool parameter:
> We're trying to avoid adding boolean parameters, because they are not very
> clear (when reading code like "hitTestResultAtPoint(point, true, false,
> true)",
> what do the booleans mean?). One strategy to replace a boolean parameter is
> to
> use a two-value enum, like this:
>
> enum HitTestScrollbars { ShouldHitTestScrollbars, DontHitTestScrollbars };
For what it is worth, I agree with David on this. It definitely makes sense and since we are a new port, let's move in a better direction, especially if the rest of WebKit is going that way.
Let's also maybe keep it in mind for the browser code too.
Regards,
Ryan
(In reply to comment #35)
> (In reply to comment #34)
> >
> > This is a new style that WebKit is moving to that likely post dates the other
> > implementations, so that's why they use an enum.
> >
> > For example see https://bugs.webkit.org/show_bug.cgi?id=26695, where this was
> > said when someone was adding a new bool parameter:
> > We're trying to avoid adding boolean parameters, because they are not very
> > clear (when reading code like "hitTestResultAtPoint(point, true, false,
> > true)",
> > what do the booleans mean?). One strategy to replace a boolean parameter is
> > to
> > use a two-value enum, like this:
> >
> > enum HitTestScrollbars { ShouldHitTestScrollbars, DontHitTestScrollbars };
>
> For what it is worth, I agree with David on this. It definitely makes sense and
> since we are a new port, let's move in a better direction, especially if the
> rest of WebKit is going that way.
>
> Let's also maybe keep it in mind for the browser code too.
>
> Regards,
> Ryan
In fact I didn't intend to do the same for the whole project. :)
Using enum instead of booleans seems totally logical to the
understanding of everyone. And I agree with it.
But here the boolean was required by the base class. (Clipboard)
Maxime
Created attachment 33166[details]
Patch to add a first bunch of Haiku-specific files for WebCore.
+#include <support/Locker.h>
+#include <app/Clipboard.h>
I have no choice but to put these includes in this order.
Clipboard.h needs Locker.h but it seems that it doesn't include it.
Comment on attachment 33166[details]
Patch to add a first bunch of Haiku-specific files for WebCore.
I want to approve this but this change isn't done.
diff --git a/WebCore/platform/haiku/CursorHaiku.cpp b/WebCore/platform/haiku/CursorHaiku.cpp
> +static Cursor global_cursor(static_cast<PlatformCursor>(B_CURSOR_SYSTEM_DEFAULT));
> +static Cursor ibeam_cursor(static_cast<PlatformCursor>(B_CURSOR_I_BEAM));
As noted before: Improper casing
s/global_cursor/globalCursor/
s/ibeam_cursor/ibeamCursor/
(In reply to comment #38)
> (From update of attachment 33166[details])
> I want to approve this but this change isn't done.
>
> diff --git a/WebCore/platform/haiku/CursorHaiku.cpp
> b/WebCore/platform/haiku/CursorHaiku.cpp
> > +static Cursor global_cursor(static_cast<PlatformCursor>(B_CURSOR_SYSTEM_DEFAULT));
> > +static Cursor ibeam_cursor(static_cast<PlatformCursor>(B_CURSOR_I_BEAM));
>
> As noted before: Improper casing
> s/global_cursor/globalCursor/
> s/ibeam_cursor/ibeamCursor/
Okay I see the issue:
I read "improper casting" but it was "improper casing".
Really sorry about that.
Comment on attachment 33168[details]
Patch to add a first bunch of Haiku-specific files for WebCore.
> +Cursor& Cursor::operator=(const Cursor& other)
> +{
> + m_impl = other.m_impl;
> + return* this;
Should be "return *this;"
Fix on landing.
Comment on attachment 32869[details]
Patch to add a fourth bunch of Haiku-specific files for WebCore.
Just a few things to fix up here.
Please run check-webkit-style on this files in this patch to point out the TABs and unsorted header files, etc. Since the tool will point them out, I'll not enumerate these issues.
> diff --git a/WebCore/platform/haiku/PasteboardHaiku.cpp b/WebCore/platform/haiku/PasteboardHaiku.cpp> +void Pasteboard::writeSelection(Range* selectedRange, bool canSmartCopyOrDelete, Frame* frame)
> +{
> + BClipboard clipboard("WebKit");
> + BMessage* data;
> +
> + if (clipboard.Lock()) {
fwiw, WebKit tends to favor a return early, so
if (!clipboard.Lock())
return;
would follow this style better.
> + clipboard.Clear();
It is nice to define types in the smallest possible scope and to initialize them when declared.
So I'd suggestion move BMessage* here.
BMessage* data = clipboard.Data();
if (data) {
Same thing for other similar code in here.
>
> diff --git a/WebCore/platform/haiku/PlatformKeyboardEventHaiku.cpp b/WebCore/platform/haiku/PlatformKeyboardEventHaiku.cpp
> + //case B_SPACE:
> + // return "";
> + case B_TAB:
> + return "U+0009";
> + //case B_ESCAPE:
> + // return "";
Typical WebKit style is to not check in commented out code.
> + return( 0 );
Just "return 0;"
> + default:
> + return( 0 );
Just "return 0;"
> +PlatformKeyboardEvent::PlatformKeyboardEvent(BMessage *message)
"*" placement. "BMessage*"
> diff --git a/WebCore/platform/haiku/PlatformWheelEventHaiku.cpp b/WebCore/platform/haiku/PlatformWheelEventHaiku.cpp
> +PlatformWheelEvent::PlatformWheelEvent(BMessage *message)
"*" placement. "BMessage*"
(In reply to comment #46)
> (From update of attachment 32869[details])
> Just a few things to fix up here.
>
>
> Please run check-webkit-style on this files in this patch to point out the TABs
> and unsorted header files, etc. Since the tool will point them out, I'll not
> enumerate these issues.
I made this patch 3 weeks ago and, during this time, I checked the style of all my files. :) Anyway, I re-ran it. Concerning the alphabetic order of headers in PlatformKeyboardEventHaiku.cpp I had no choices.
Comment on attachment 34263[details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
> +#include "DocumentFragment.h"
> +#include "Editor.h"
> +#include "Frame.h"
> +#include "KURL.h"
> +#include "NotImplemented.h"
> +#include "markup.h"
> +
> +#include <support/Locker.h>
> +#include <Clipboard.h>
> +#include <Message.h>
> +#include <String.h>
WebKit style is to use a single paragraph for these, not two.
> +#include "KeyboardCodes.h"
> +#include "NotImplemented.h"
> +
> +#include <InterfaceDefs.h>
> +#include <Message.h>
> +#include <String.h>
Ditto.
> +static String keyIdentifierForHaikuKeyCode(char singleByte, int32 keyCode)
Are you sure you want to use the type "int32" here? We'd normally use int, or perhaps int32_t. Is this something specific to the Haiku platform?
> +{
> + switch (singleByte) {
This is indented two many spaces.
> + return String::format("U+%04X", toASCIIUpper((int)keyCode));
You should not have to use any type cast with toASCIIUpper. Does this help in some way?
> + };
Extra semicolon here.
> + return String::format("U+%04X", toASCIIUpper((int)keyCode));
If you put this after the switch statement then you could share the two cases of it.
> + switch (singleByte) {
For some reason this second switch statement is indented differently than the first. Could you make both consistent, and follow the WebKit coding style guide?
> +PlatformKeyboardEvent::PlatformKeyboardEvent(BMessage* message)
> +{
> + BString bytes = message->FindString("bytes");
> +
> + m_text = String::fromUTF8(bytes.String(), bytes.Length());
> + m_unmodifiedText = String(bytes.String(), bytes.Length());
> + m_keyIdentifier = keyIdentifierForHaikuKeyCode(bytes.ByteAt(0), message->FindInt32("key"));
> +
> + if (message->what == B_KEY_UP)
> + m_type = KeyUp;
> + else if (message->what == B_KEY_DOWN)
> + m_type = KeyDown;
> +
> + m_autoRepeat = true;
> + m_ctrlKey = false;
> + m_altKey = false;
> + m_metaKey = false;
> + m_windowsVirtualKeyCode = windowsKeyCodeForKeyEvent(bytes.ByteAt(0));
> + m_isKeypad = false;
> + m_shiftKey = false;
> +}
It's better style to use member initialization as much as possible, and only use assignment when that is impractical. So at least for most of these booleans you should switch to that.
> +PlatformMouseEvent::PlatformMouseEvent(const BMessage* message)
Same comment about member-wise initialization for this constructor.
> + m_deltaX *= -static_cast<float>(cScrollbarPixelsPerLineStep);
> + m_deltaY *= -static_cast<float>(cScrollbarPixelsPerLineStep);
Is the cast to float needed? What effect does it have? I'd expect this to work perfectly without the cast.
review- because there are enough comments above that could be simply addressed
Comment on attachment 34263[details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
> +#include "DocumentFragment.h"
> +#include "Editor.h"
> +#include "Frame.h"
> +#include "KURL.h"
> +#include "NotImplemented.h"
> +#include "markup.h"
> +
> +#include <support/Locker.h>
> +#include <Clipboard.h>
> +#include <Message.h>
> +#include <String.h>
WebKit style is to use a single paragraph for these, not two.
> +#include "KeyboardCodes.h"
> +#include "NotImplemented.h"
> +
> +#include <InterfaceDefs.h>
> +#include <Message.h>
> +#include <String.h>
Ditto.
> +static String keyIdentifierForHaikuKeyCode(char singleByte, int32 keyCode)
Are you sure you want to use the type "int32" here? We'd normally use int, or perhaps int32_t. Is this something specific to the Haiku platform?
> +{
> + switch (singleByte) {
This is indented two many spaces.
> + return String::format("U+%04X", toASCIIUpper((int)keyCode));
You should not have to use any type cast with toASCIIUpper. Does this help in some way?
> + };
Extra semicolon here.
> + return String::format("U+%04X", toASCIIUpper((int)keyCode));
If you put this after the switch statement then you could share the two cases of it.
> + switch (singleByte) {
For some reason this second switch statement is indented differently than the first. Could you make both consistent, and follow the WebKit coding style guide?
> +PlatformKeyboardEvent::PlatformKeyboardEvent(BMessage* message)
> +{
> + BString bytes = message->FindString("bytes");
> +
> + m_text = String::fromUTF8(bytes.String(), bytes.Length());
> + m_unmodifiedText = String(bytes.String(), bytes.Length());
> + m_keyIdentifier = keyIdentifierForHaikuKeyCode(bytes.ByteAt(0), message->FindInt32("key"));
> +
> + if (message->what == B_KEY_UP)
> + m_type = KeyUp;
> + else if (message->what == B_KEY_DOWN)
> + m_type = KeyDown;
> +
> + m_autoRepeat = true;
> + m_ctrlKey = false;
> + m_altKey = false;
> + m_metaKey = false;
> + m_windowsVirtualKeyCode = windowsKeyCodeForKeyEvent(bytes.ByteAt(0));
> + m_isKeypad = false;
> + m_shiftKey = false;
> +}
It's better style to use member initialization as much as possible, and only use assignment when that is impractical. So at least for most of these booleans you should switch to that.
> +PlatformMouseEvent::PlatformMouseEvent(const BMessage* message)
Same comment about member-wise initialization for this constructor.
> + m_deltaX *= -static_cast<float>(cScrollbarPixelsPerLineStep);
> + m_deltaY *= -static_cast<float>(cScrollbarPixelsPerLineStep);
Is the cast to float needed? What effect does it have? I'd expect this to work perfectly without the cast.
review- because there are enough comments above that could be simply addressed
> > +static String keyIdentifierForHaikuKeyCode(char singleByte, int32 keyCode)
>
> Are you sure you want to use the type "int32" here? We'd normally use int, or
> perhaps int32_t. Is this something specific to the Haiku platform?
>
int32 is indeed a specific int type of Haiku. I change it to int.
> > + m_deltaX *= -static_cast<float>(cScrollbarPixelsPerLineStep);
> > + m_deltaY *= -static_cast<float>(cScrollbarPixelsPerLineStep);
>
> Is the cast to float needed? What effect does it have? I'd expect this to work
> perfectly without the cast.
>
The cast isn't necessary, even if cScrollbarPixelsPerLineStep is an int.
Anyway, other ports do this cast. (I removed it in this patch.)
Comment on attachment 32875[details]
Patch to add a fifth bunch of Haiku-specific files for WebCore.
Here are some other things to fix up in the code. I gave examples in places but the whole patch should be examined for the issues.
Needs a check-webkit-style pass.
Also, look at spacing. There are several places. Here's an example:
110 BView* view=i.context->platformContext();
No spaces around =. To much space after BView* (actually it may be a TAB).
112 if (isEnabled( o )) {
No spaces to the left of ( or to the right of ).
Odd indentation:
170 view->SetHighColor(tint_color(ui_color(B_KEYBOARD_NAVIGATION_COLOR),
179 B_DISABLED_MARK_TINT));
Typically indented to the nested ( on the previous line.
182 // needed because of anti-aliasing
The comment should be aligned with the code and be formed like a sentence (starting with a capital and ending with a period).
183 view->StrokeLine(BPoint(rect.left, rect.top),
Variable names should be words. Example: "const RenderObject::PaintInfo& i"
" //code taken from the Haiku source code (CheckBox.cpp)"
What is the license on the Haiku source? Does it need to be added to the file and is it WebKit compatible?
> " //code taken from the Haiku source code (CheckBox.cpp)"
> What is the license on the Haiku source? Does it need to be added to the file
> and is it WebKit compatible?
Haiku uses a BSD License for its code. So I don't think we should add the license.
I think this comment was simply to signify that it's a copy/paste (I didn't write it).
(In reply to comment #53)
> (From update of attachment 32875[details])
> Here are some other things to fix up in the code. I gave examples in places
> but the whole patch should be examined for the issues.
>
> Needs a check-webkit-style pass.
>
I made a check-webkit-style and I got only one issue because of headers that I can't put in the correct order. :)
(In reply to comment #55)
> (In reply to comment #53)
> > (From update of attachment 32875[details] [details])
> > Here are some other things to fix up in the code. I gave examples in places
> > but the whole patch should be examined for the issues.
> >
> > Needs a check-webkit-style pass.
> >
>
> I made a check-webkit-style and I got only one issue because of headers that I
> can't put in the correct order. :)
That's odd. There were several places with TABs and the use of NULL. Both should have been flagged.
(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #53)
> > > (From update of attachment 32875[details] [details] [details])
> > > Here are some other things to fix up in the code. I gave examples in places
> > > but the whole patch should be examined for the issues.
> > >
> > > Needs a check-webkit-style pass.
> > >
> >
> > I made a check-webkit-style and I got only one issue because of headers that I
> > can't put in the correct order. :)
>
> That's odd. There were several places with TABs and the use of NULL. Both
> should have been flagged.
That's simply because I already reviewed these files and didn't updated the patches.
Comment on attachment 34301[details]
Patch to add a sixth bunch of Haiku-specfic files to WebCore.
Again, everything but translator decoder looks fine. too much stuff in one patch.
Please get pkasting on #webkit to look at TranslatorDecoder as he knows the most about the image decoders.
Comment on attachment 34291[details]
Patch to add a fifth bunch of Haiku-specfic files to WebCore.
Too large. Please break this up. Why are we still using this one bug?
Does Haiku really not have any drawing routines for drawing a checkbox or a radio button?
(In reply to comment #62)
> (From update of attachment 34291[details])
> Too large. Please break this up. Why are we still using this one bug?
Yes please open new bugs Maxime with more specific descriptions and patches that cover fairly self-contained areas.
> Does Haiku really not have any drawing routines for drawing a checkbox or a
> radio button?
It does now, but probably not when this code was originally written. This is my fault for not being more active in updating this code and advising Maxime on what needed to be updated. I apologize for that and for all the review problems in these patches.
> I apologize for that and for all the review problems in these patches.
fwiw, I think we all go through this when working in a new code base. I wouldn't take it too hard but it is good to learn from the reviews and improve current and future patches, which I think y'all are doing.
Comment on attachment 34278[details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
Clearing review flag on attachment: 34278
Committing to http://svn.webkit.org/repository/webkit/trunk ...
M WebCore/ChangeLog
A WebCore/platform/haiku/PasteboardHaiku.cpp
A WebCore/platform/haiku/PlatformKeyboardEventHaiku.cpp
A WebCore/platform/haiku/PlatformMouseEventHaiku.cpp
A WebCore/platform/haiku/PlatformWheelEventHaiku.cpp
Committed r46934
M WebCore/ChangeLog
A WebCore/platform/haiku/PasteboardHaiku.cpp
A WebCore/platform/haiku/PlatformMouseEventHaiku.cpp
A WebCore/platform/haiku/PlatformKeyboardEventHaiku.cpp
A WebCore/platform/haiku/PlatformWheelEventHaiku.cpp
r46934 = 2a4385cbc2a85dbec8738df602dcaf69c09f4f11 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46934
2009-07-06 06:19 PDT, Maxime Simon
2009-07-06 06:26 PDT, Maxime Simon
2009-07-06 06:34 PDT, Maxime Simon
2009-07-06 06:45 PDT, Maxime Simon
2009-07-06 07:12 PDT, Maxime Simon
2009-07-06 08:21 PDT, Maxime Simon
2009-07-06 08:35 PDT, Maxime Simon
2009-07-16 05:21 PDT, Maxime Simon
2009-07-16 06:43 PDT, Maxime Simon
2009-07-16 06:53 PDT, Maxime Simon
2009-07-16 07:22 PDT, Maxime Simon
2009-07-16 09:01 PDT, Maxime Simon
2009-07-16 09:15 PDT, Maxime Simon
2009-07-17 07:55 PDT, Maxime Simon
2009-07-17 08:29 PDT, Maxime Simon
2009-07-20 10:28 PDT, Maxime Simon
2009-07-21 02:47 PDT, Maxime Simon
2009-07-21 03:26 PDT, Maxime Simon
2009-08-07 02:46 PDT, Maxime Simon
2009-08-07 08:23 PDT, Maxime Simon
2009-08-07 10:42 PDT, Maxime Simon
2009-08-07 11:39 PDT, Maxime Simon