Bug 26988

Summary: Haiku-specific files for WebCore
Product: WebKit Reporter: Maxime Simon <simon.maxime>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, leavengood, levin, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Other   
Attachments:
Description Flags
Patch to add a first bunch of Haiku-specific files for WebCore.
oliver: review-
Patch to add a second bunch of Haiku-specific files for WebCore.
none
Patch to add a third bunch of Haiku-specific files for WebCore.
none
Patch to add a fourth bunch of Haiku-specific files for WebCore.
oliver: review-
Patch to add ScrollBar and ScrollView code specific to Haiku.
oliver: review-
Patch to add a fifth bunch of Haiku-specific files for WebCore.
oliver: review-
Patch to add a sixth bunch of Haiku-specific files for WebCore.
oliver: review-
Patch to add a first bunch of Haiku-specific files for WebCore.
none
Patch to add a first bunch of Haiku-specific files for WebCore.
levin: review-
Patch to add a fourth bunch of Haiku-specific files for WebCore.
levin: review-
Patch to add ScrollBar and ScrollView code specific to Haiku.
none
Patch to add a fifth bunch of Haiku-specific files for WebCore.
levin: review-
Patch to add a sixth bunch of Haiku-specific files for WebCore.
none
Patch to add ScrollBar and ScrollView code specific to Haiku.
none
Patch to add ScrollBar and ScrollView code specific to Haiku.
none
Patch to add a first bunch of Haiku-specific files for WebCore.
levin: review-
Patch to add a first bunch of Haiku-specific files for WebCore.
levin: review-
Patch to add a first bunch of Haiku-specific files for WebCore.
levin: review-
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
darin: review-
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
none
Patch to add a fifth bunch of Haiku-specfic files to WebCore.
eric: review-
Patch to add a sixth bunch of Haiku-specfic files to WebCore. eric: review-

Description Maxime Simon 2009-07-06 06:04:18 PDT
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 1 Maxime Simon 2009-07-06 06:19:49 PDT
Created attachment 32300 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.
Comment 2 Maxime Simon 2009-07-06 06:26:11 PDT
Created attachment 32301 [details]
Patch to add a second bunch of Haiku-specific files for WebCore.
Comment 3 Maxime Simon 2009-07-06 06:34:04 PDT
Created attachment 32302 [details]
Patch to add a third bunch of Haiku-specific files for WebCore.
Comment 4 Maxime Simon 2009-07-06 06:45:38 PDT
Created attachment 32303 [details]
Patch to add a fourth bunch of Haiku-specific files for WebCore.
Comment 5 Maxime Simon 2009-07-06 07:12:03 PDT
Created attachment 32304 [details]
Patch to add ScrollBar and ScrollView code specific to Haiku.
Comment 6 Maxime Simon 2009-07-06 08:21:41 PDT
Created attachment 32307 [details]
Patch to add a fifth bunch of Haiku-specific files for WebCore.
Comment 7 Maxime Simon 2009-07-06 08:35:55 PDT
Created attachment 32308 [details]
Patch to add a sixth bunch of Haiku-specific files for WebCore.
Comment 8 Oliver Hunt 2009-07-16 00:23:41 PDT
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 9 Oliver Hunt 2009-07-16 00:26:02 PDT
Comment on attachment 32301 [details]
Patch to add a second bunch of Haiku-specific files for WebCore.

looks fine
Comment 10 Oliver Hunt 2009-07-16 00:28:26 PDT
Comment on attachment 32302 [details]
Patch to add a third bunch of Haiku-specific files for WebCore.

looks fine
Comment 11 Oliver Hunt 2009-07-16 00:36:51 PDT
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
Comment 12 Oliver Hunt 2009-07-16 00:43:56 PDT
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
Comment 13 Oliver Hunt 2009-07-16 00:49:50 PDT
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
Comment 14 Oliver Hunt 2009-07-16 00:53:48 PDT
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 {
Comment 15 Oliver Hunt 2009-07-16 00:54:20 PDT
In general all these patches look fine, it's just minor style quirks holding them up from being landed
Comment 16 Maxime Simon 2009-07-16 05:21:47 PDT
Created attachment 32861 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.
Comment 17 Maxime Simon 2009-07-16 06:24:49 PDT
(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.
Comment 18 Maxime Simon 2009-07-16 06:43:32 PDT
Created attachment 32868 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.

Sorry, the previous patch lacks some files.
Comment 19 Maxime Simon 2009-07-16 06:53:44 PDT
Created attachment 32869 [details]
Patch to add a fourth bunch of Haiku-specific files for WebCore.
Comment 20 Maxime Simon 2009-07-16 07:22:55 PDT
Created attachment 32870 [details]
Patch to add ScrollBar and ScrollView code specific to Haiku.
Comment 21 Maxime Simon 2009-07-16 09:01:18 PDT
Created attachment 32875 [details]
Patch to add a fifth bunch of Haiku-specific files for WebCore.
Comment 22 Maxime Simon 2009-07-16 09:15:18 PDT
Created attachment 32876 [details]
Patch to add a sixth bunch of Haiku-specific files for WebCore.
Comment 23 Oliver Hunt 2009-07-16 12:07:38 PDT
> > 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
Comment 24 Maxime Simon 2009-07-16 12:31:46 PDT
(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 25 Adam Barth 2009-07-16 23:41:45 PDT
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 26 Adam Barth 2009-07-16 23:42:03 PDT
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
Comment 27 Adam Barth 2009-07-16 23:42:07 PDT
All reviewed patches landed, closing.
Comment 28 Adam Barth 2009-07-16 23:42:44 PDT
bugzilla requires a comment here
Comment 29 Maxime Simon 2009-07-17 07:55:27 PDT
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 :)
Comment 30 Maxime Simon 2009-07-17 08:29:46 PDT
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 31 David Levin 2009-07-19 23:37:10 PDT
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.
Comment 32 Maxime Simon 2009-07-20 10:28:06 PDT
Created attachment 33093 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.
Comment 33 Maxime Simon 2009-07-20 10:37:01 PDT
(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 34 David Levin 2009-07-20 10:56:04 PDT
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
Comment 35 Ryan Leavengood 2009-07-20 13:19:57 PDT
(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
Comment 36 Maxime Simon 2009-07-20 14:06:16 PDT
(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
Comment 37 Maxime Simon 2009-07-21 02:47:20 PDT
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 38 David Levin 2009-07-21 03:03:19 PDT
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/
Comment 39 Maxime Simon 2009-07-21 03:11:19 PDT
(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 40 Maxime Simon 2009-07-21 03:26:51 PDT
Created attachment 33168 [details]
Patch to add a first bunch of Haiku-specific files for WebCore.
Comment 41 David Levin 2009-07-21 03:37:34 PDT
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 42 David Levin 2009-07-21 03:42:12 PDT
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
Comment 43 David Levin 2009-07-28 12:17:39 PDT
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.
Comment 44 Maxime Simon 2009-07-28 12:19:46 PDT
(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)
Comment 45 David Levin 2009-07-28 13:34:32 PDT
>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.
Comment 46 David Levin 2009-08-07 01:59:46 PDT
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*"
Comment 47 Maxime Simon 2009-08-07 02:44:53 PDT
(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 48 Maxime Simon 2009-08-07 02:46:26 PDT
Created attachment 34263 [details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
Comment 49 Darin Adler 2009-08-07 07:42:21 PDT
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 50 Darin Adler 2009-08-07 07:42:26 PDT
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 51 Maxime Simon 2009-08-07 08:23:20 PDT
Created attachment 34278 [details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.
Comment 52 Maxime Simon 2009-08-07 08:26:18 PDT
> > +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 53 David Levin 2009-08-07 10:05:06 PDT
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?
Comment 54 Maxime Simon 2009-08-07 10:10:44 PDT
> " //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).
Comment 55 Maxime Simon 2009-08-07 10:25:18 PDT
(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. :)
Comment 56 David Levin 2009-08-07 10:28:51 PDT
(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.
Comment 57 Maxime Simon 2009-08-07 10:31:58 PDT
(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 58 Maxime Simon 2009-08-07 10:42:01 PDT
Created attachment 34291 [details]
Patch to add a fifth bunch of Haiku-specfic files to WebCore.
Comment 59 Maxime Simon 2009-08-07 11:39:46 PDT
Created attachment 34301 [details]
Patch to add a sixth bunch of Haiku-specfic files to WebCore.
Comment 60 Eric Seidel (no email) 2009-08-07 12:16:56 PDT
Comment on attachment 34278 [details]
Patch to add a fourth bunch of Haiku-specfic files to WebCore.

Looks sane enough.
Comment 61 Eric Seidel (no email) 2009-08-07 12:18:50 PDT
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 62 Eric Seidel (no email) 2009-08-07 12:20:48 PDT
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?
Comment 63 Ryan Leavengood 2009-08-07 16:13:27 PDT
(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.
Comment 64 David Levin 2009-08-07 16:23:38 PDT
>  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 65 Adam Barth 2009-08-07 17:48:21 PDT
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
Comment 66 Maxime Simon 2009-08-15 03:11:09 PDT
Non need to keep this bug opened.