Bug 26988 – Haiku-specific files for WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Maxime Simon]

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 )

[Maxime Simon]

Created attachment 32300 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32301 [details]
Patch to add a second bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32302 [details]
Patch to add a third bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32303 [details]
Patch to add a fourth bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32304 [details]
Patch to add ScrollBar and ScrollView code specific to Haiku.

[Maxime Simon]

Created attachment 32307 [details]
Patch to add a fifth bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32308 [details]
Patch to add a sixth bunch of Haiku-specific files for WebCore.

[Oliver Hunt]

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

[Oliver Hunt]

Comment on attachment 32301 [details]
Patch to add a second bunch of Haiku-specific files for WebCore.

looks fine

[Oliver Hunt]

Comment on attachment 32302 [details]
Patch to add a third bunch of Haiku-specific files for WebCore.

looks fine

[Oliver Hunt]

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

[Oliver Hunt]

Comment on attachment 32304 [details]
Patch to add ScrollBar and ScrollView code specific to Haiku.

> +
> +void ScrollViewCanvasHaiku::Draw(BRect updateRect)
> +{
> +    FrameView* fv = static_cast<FrameView*>(m_scrollView);
> +    if (!fv || !fv->frame() || !fv->frame()->contentRenderer())
> +        return;
> +
> +    while (fv->needsLayout()) {
> +        fv->layout();
> +    }
Single line code block should not have braces :D

> +void ScrollViewCanvasHaiku::KeyDown(const char* bytes, int32 numBytes)
> +{
> +    BMessage* message = Looper()->CurrentMessage();
> +    if (!message)
> +        return;
> +
> +    WebCore::FrameView* fv = static_cast<WebCore::FrameView*>(m_scrollView);
> +    if (!fv || !fv->frame())
> +    	return;
> +
> +    if(!fv->frame()->eventHandler()->keyEvent(PlatformKeyboardEvent(message)))
> +    	printf("-->KeyDown: not arrived\n");    
> +
> +    if((int)bytes[0] != (int)B_TAB)
> +        BView::KeyDown(bytes, numBytes);

We use c++ casts for all casts in webkit -- eg. static_cast

> +}
> +
> +void ScrollViewCanvasHaiku::KeyUp(const char* bytes, int32 numBytes)
> +{
> +    BMessage* message = Looper()->CurrentMessage();
> +    if (!message)
> +        return;
> +
> +    WebCore::FrameView* fv = static_cast<WebCore::FrameView*>(m_scrollView);
> +    if (!fv || !fv->frame())
> +    	return;
> +
> +    if(fv->frame()->eventHandler()->keyEvent(PlatformKeyboardEvent(message)))
> +    	printf("-->KeyUp: not arrived\n"); 
> +
> +    if(bytes[0] != (int)B_TAB)
ditto

Basically looks fine except the aforementioned style issues

[Oliver Hunt]

Comment on attachment 32307 [details]
Patch to add a fifth bunch of Haiku-specific files for WebCore.


> +// This might be useful, but it not used for now
> +/*
> +static BView* haikuWidgetForPage(const Page* page)
> +{
> +    Frame* frame = (page ? page->mainFrame() : 0);
> +    FrameView* frameView = (frame ? frame->view() : 0);
> +
> +    if (!frameView)
> +        return 0;
> +
> +    return frameView->haikuWidget();
> +}*/

We don't include commented out code in the tree


> +int screenDepth(Widget*)
> +{
> +    BScreen screen;
> +    // FIXME: We assume this screen is valid
> +    color_space cs = screen.ColorSpace();
> +
> +    size_t pixelChunk, rowAlignment, pixelsPerChunk;
> +    if (get_pixel_size_for(cs, &pixelChunk, &rowAlignment, &pixelsPerChunk) == B_OK) {
> +        // FIXME: Not sure if this is right
> +        return pixelChunk * 8;
> +    }
Single line block shouldn't have braces

> +void SharedTimerHaiku::setFireTime(double fireTime)
> +{
> +    m_fireTime = fireTime;
> +    if (m_fireTime > 0 && m_shouldRun) {
> +        bigtime_t sleepyTime = (bigtime_t)((m_fireTime - WTF::currentTime()) * 1000000);
You should be using a c++ style cast

> +        
> +        if(sleepyTime < 0)
space between if and (

> +// WebCore functions
> +void setSharedTimerFiredFunction(void (*function)())
> +{
> +    SharedTimerHaiku* timer=getSharedTimerHaiku();
Need spaces around =

> +namespace WebCore {
> +    class SharedTimerHaiku : public BMessageFilter {
> +        public:
> +            SharedTimerHaiku();
> +            ~SharedTimerHaiku();
> +
> +            void setFireTime(double fireTime);
> +            void setFireFunction(void (*function)());
> +            void setShouldRun(bool shouldRun);
> +
> +            //void  MessageReceived(BMessage* message);
> +            virtual filter_result Filter(BMessage* message, BHandler** _target);
> +
> +        private:
> +            //void wait();
> +            //static int32 threadFunction(void *data);
> +
> +            //thread_id m_thread;
> +            //sem_id m_waitSem;
Don't included commented out members, you should file a bug on resurrecting these

[Oliver Hunt]

Comment on attachment 32308 [details]
Patch to add a sixth bunch of Haiku-specific files for WebCore.


> +double currentTime()
> +{
> +    struct timeval tv;
> +    struct timezone tz;
> +
> +    gettimeofday(&tv, &tz);
> +    return (double)tv.tv_sec + (double)(tv.tv_usec / 1000000.0);
Once again c++ style casts please :D

> +TranslatorDecoder::TranslatorDecoder()
> +{
> +    m_image = 0;
> +    setSize(-1,-1);
> +}
space after the ,

> +
> +bool TranslatorDecoder::isSizeAvailable() const
> +{
> +    return !(size() == IntSize(-1, -1));
> +}
Does != really not work here?

> +void TranslatorDecoder::setData(SharedBuffer* data, bool allDataReceived) 
> +{ 
> +    if(allDataReceived) {
> +        BMemoryIO io(data->data(),data->size());
> +        m_image = BTranslationUtils::GetBitmap(&io);
> +        if (m_image) {
> +            //set the image size decoded.
> +            setSize(m_image->Bounds().Width() + 1, m_image->Bounds().Height() + 1);
> +        }   
Should not have braces around a single line if :D

> +void Widget::setFrameRect(const IntRect& rect)
> +{
> +    if (!platformWidget())
> +        return;
> +
> +    if (frameRect() != rect){
Space before {

[Oliver Hunt]

In general all these patches look fine, it's just minor style quirks holding them up from being landed

[Maxime Simon]

Created attachment 32861 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

[Maxime Simon]

(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.

[Maxime Simon]

Created attachment 32868 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

Sorry, the previous patch lacks some files.

[Maxime Simon]

Created attachment 32869 [details]
Patch to add a fourth bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32870 [details]
Patch to add ScrollBar and ScrollView code specific to Haiku.

[Maxime Simon]

Created attachment 32875 [details]
Patch to add a fifth bunch of Haiku-specific files for WebCore.

[Maxime Simon]

Created attachment 32876 [details]
Patch to add a sixth bunch of Haiku-specific files for WebCore.

[Oliver Hunt]

> > 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

[Maxime Simon]

(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.

[Adam Barth]

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

[Adam Barth]

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

[Adam Barth]

All reviewed patches landed, closing.

[Adam Barth]

bugzilla requires a comment here

[Maxime Simon]

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 :)

[Maxime Simon]

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.

[David Levin]

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.

[Maxime Simon]

Created attachment 33093 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

[Maxime Simon]

(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.

[David Levin]

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

[Ryan Leavengood]

(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

[Maxime Simon]

(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

[Maxime Simon]

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.

[David Levin]

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/

[Maxime Simon]

(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.

[Maxime Simon]

Created attachment 33168 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

[David Levin]

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.

[David Levin]

Comment on attachment 33168 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

Clearing r+.
Committed as http://trac.webkit.org/changeset/46167

[David Levin]

Comment on attachment 33168 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

I believe I already landed this: http://trac.webkit.org/changeset/46167

Let me know if I am mistaken.

[Maxime Simon]

(In reply to comment #43)
> (From update of attachment 33168 [details])
> I believe I already landed this: http://trac.webkit.org/changeset/46167
> 
> Let me know if I am mistaken.

Indeed, strange that it was still highlighted. (with no flags)

[David Levin]

>Indeed, strange that it was still highlighted. (with no flags)
I did that.

In short: Best to stick with one patch per bug (but when you're starting with WebKit, it is hard to realize that :) ).


Details:
There are several problems when putting multiple patches in one bug.

One problem is the bugs need to remain open even after one of the patches is landed.  However, if there is an r+ in an open bug, then the bug shows up in the "commit queue": 

https://bugs.webkit.org/buglist.cgi?query_format=advanced&short_desc_type=notregexp&short_desc=%5C%5BS60%5C%5D&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&keywords_type=allwords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review%2B&field0-1-0=noop&type0-1-0=equals&value0-1-0=

So it looks like the bug needs to have a patch committed (but it doesn't).  To solve this, the r+ is cleared for landed patches in open bugs to remove it from the commit queue. There are mixed opinions on whether the patch should be marked obsolete.  I didn't because the patch was fine.  This is what you saw here :)


btw, another problem with multiple patches per bug is trying to figure out what comments apply to what patch.

[David Levin]

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*"

[Maxime Simon]

(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.

[Maxime Simon]

Created attachment 34263 [details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.

[Darin Adler]

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

[Darin Adler]

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

[Maxime Simon]

Created attachment 34278 [details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.

[Maxime Simon]

> > +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.)

[David Levin]

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?

[Maxime Simon]

> " //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).

[Maxime Simon]

(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. :)

[David Levin]

(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.

[Maxime Simon]

(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.

[Maxime Simon]

Created attachment 34291 [details]
Patch to add a fifth bunch of Haiku-specfic files to WebCore.

[Maxime Simon]

Created attachment 34301 [details]
Patch to add a sixth bunch of Haiku-specfic files to WebCore.

[Eric Seidel (no email)]

Comment on attachment 34278 [details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.

Looks sane enough.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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?

[Ryan Leavengood]

(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.

[David Levin]

>  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.

[Adam Barth]

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

[Maxime Simon]

Non need to keep this bug opened.