Bug 12160 – Linux/Gdk build fixes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Krzysztof Kowalczyk]

Linux/Gdk build fixes.

[Krzysztof Kowalczyk]

Created attachment 12297 [details]
linux\gdk build fixes

[Krzysztof Kowalczyk]

Created attachment 12354 [details]
Updated linux/gtk patch to r18755

[Krzysztof Kowalczyk]

Created attachment 12527 [details]
Updated linux/gdk fixes to r18930

[Krzysztof Kowalczyk]

Created attachment 12835 [details]
linux\gdk build fixes

[Krzysztof Kowalczyk]

Created attachment 12989 [details]
Linux\gdk build fixes - el patch numero uno

First in a series of small patches fixing gdk/linux build.

[Krzysztof Kowalczyk]

Created attachment 12990 [details]
Linux\gdk build fixes - el patch numero dos

Second in a series of small patches for fixing linux\gdk build.

[Krzysztof Kowalczyk]

Created attachment 12991 [details]
Linux\gdk build fixes - el patch numero treis

Third in a series of small patches for fixing linux\gdk build. Those are related to graphics.

[Krzysztof Kowalczyk]

Created attachment 12992 [details]
Linux\gdk build fixes - el patch numero quatro

Fourth in a series of small patches for fixing linux/gdk build - those are related to font handling.

[Krzysztof Kowalczyk]

Created attachment 12993 [details]
Linux\gdk build fixes - el patch numero cinco

Fifth in a series of small patches for fixing linux/gdk build - add DragDataGdk.cpp.

[Krzysztof Kowalczyk]

Created attachment 12994 [details]
Linux\gdk build fixes - el patch numero seis

Fix crash on gdk build due to mismatched type of alloc/free function (allocs with fastAlloc() but frees with regular free())

[Krzysztof Kowalczyk]

Created attachment 12995 [details]
Linux\gdk build fixes - el patch numero siete

Seventh in a series of small patches for fixing linux/gdk build - functions related to networking.

[Maciej Stachowiak]

Krzysztof: thanks for breaking it up! You are my hero.

[Maciej Stachowiak]

Comment on attachment 12992 [details]
Linux\gdk build fixes - el patch numero quatro

r=me

[Adam Roben (:aroben)]

Comment on attachment 12989 [details]
Linux\gdk build fixes - el patch numero uno

+    return "Mozilla/5.0 (PC; U; Intel; Windows; en) AppleWebKit/420+ (KHTML, like Gecko)";

   "Windows" doesn't seem completely accurate here...

   It'd be nice to see this as an svn cp rather than just an svn add.

   r=me

[Adam Roben (:aroben)]

Comment on attachment 12990 [details]
Linux\gdk build fixes - el patch numero dos

+namespace WebCore
+{

   The opening brace should be on the same line as namespace WebCore.

+    // FIXME: this method always returns true
+    notImplemented();
+    return false;

   The comment here seems completely incorrect.

+bool EventHandler::handleDrag(const MouseEventWithHitTestResults& event)
+{
+    //notImplemented();
+    return false;
+}

   Why is notImplented() commented out?

   Again, svn cp would be better.

   r=me.

[Adam Roben (:aroben)]

Comment on attachment 12993 [details]
Linux\gdk build fixes - el patch numero cinco

r=me

[Maciej Stachowiak]

Comment on attachment 12993 [details]
Linux\gdk build fixes - el patch numero cinco

r=me

[Maciej Stachowiak]

Comment on attachment 12994 [details]
Linux\gdk build fixes - el patch numero seis

r=me

[Maciej Stachowiak]

Comment on attachment 12995 [details]
Linux\gdk build fixes - el patch numero siete

I'm not sure ResourceHandleManager::self() is a good name. get() was kind of vague, but a more typical name for a static method when applying the singleton pattern like this would be sharedInstance() or just shared(). Otherwise these changes look fine. r=me and feel free to adjust name when landing if you want.

[Adam Roben (:aroben)]

Comment on attachment 12991 [details]
Linux\gdk build fixes - el patch numero treis

+#include <cairo.h>
 #include "FloatRect.h"
 #include "Font.h"
+#include "FontData.h"
 #include "IntRect.h"
-#include <cairo.h>
 #include <math.h>
+#include <stdio.h>

   cairo.h should stay where it was, above math.h.

-void GraphicsContext::setFocusRingClip(const IntRect&)
+void GraphicsContext::setFocusRingClip(const IntRect& rect)

   This change is unnecessary.

+void GraphicsContext::setPlatformFillColor(const Color& col)
+{
+    setColor(m_data->context, col);
+    cairo_fill(m_data->context);

   Does cairo_fill actually do a fill? That's not what we want here, we just want to set a color for when a fill does happen later. Ditto for stroke.

+    if (m_decoder) {
+        delete  m_decoder;
+        m_decoder = 0;

   Null check is unnecessary. There's an extra space after 'delete'.

+    BitmapImage* img = new BitmapImage();

   Parentheses are unnecessary here.

   If cairo_stroke and cairo_fill don't actually do a stroke/fill, r=me.

[Krzysztof Kowalczyk]

Comment on attachment 12992 [details]
Linux\gdk build fixes - el patch numero quatro

commited

[Krzysztof Kowalczyk]

Comment on attachment 12989 [details]
Linux\gdk build fixes - el patch numero uno

commited. Used svn cp and changed user agent to "Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/420+ (KHTML, like Gecko)" as per review comments.

[Krzysztof Kowalczyk]

Comment on attachment 12990 [details]
Linux\gdk build fixes - el patch numero dos

committed, used svn cp, fixed the brace placement and removed bad comment (it's already gone in qt version), as per review comments.

[Krzysztof Kowalczyk]

Comment on attachment 12993 [details]
Linux\gdk build fixes - el patch numero cinco

commited

[Krzysztof Kowalczyk]

Comment on attachment 12994 [details]
Linux\gdk build fixes - el patch numero seis

committed

[Krzysztof Kowalczyk]

Comment on attachment 12995 [details]
Linux\gdk build fixes - el patch numero siete

commited. used sharedInstance() instead of self() as per review comments.

[Krzysztof Kowalczyk]

Comment on attachment 12991 [details]
Linux\gdk build fixes - el patch numero treis

committed ImageCairo.cpp, ImageSoruceCairo.cpp and ImageGdk.cpp making changes suggest in review. Will re-submit GraphicsContextCairo.cpp since Adam is right about cairo_fill and cairo_stroke not being used correctly.

[Krzysztof Kowalczyk]

Created attachment 13022 [details]
Linux\gdk build fixes - el patch numero huit

Eight in a series of small patches for fixing linux/gdk build.

[Krzysztof Kowalczyk]

Created attachment 13023 [details]
Linux\gdk build fixes - el patch numero nove

Ninth in the series of small patches for fixing linux/gdk build.

[Krzysztof Kowalczyk]

Created attachment 13026 [details]
linux\gdk build fixes for cairo

10th patch for linux/gdk for cairo-related stuff. The difference between those patches and the ones submitted earlier are:
* in GraphicsContext.cpp - remove functions instead of #ifdefing them out for all platforms
* in GraphicsContextCairo.cpp make setPlatformFillColor() and setPlatformStrokeColor() no-ops since Adam was right that cairo_fill and cairo_stroke shouldn't be used there (they do the actual drawing)

[Krzysztof Kowalczyk]

Created attachment 13028 [details]
ScrollView for gdk update

Add ScrollView::updateGeometry() for gdk.

[Maciej Stachowiak]

Comment on attachment 13028 [details]
ScrollView for gdk update

r=me

[Maciej Stachowiak]

Comment on attachment 13023 [details]
Linux\gdk build fixes - el patch numero nove

r=me

[Krzysztof Kowalczyk]

Comment on attachment 13023 [details]
Linux\gdk build fixes - el patch numero nove

commited

[Krzysztof Kowalczyk]

Comment on attachment 13028 [details]
ScrollView for gdk update

commited

[Krzysztof Kowalczyk]

Created attachment 13046 [details]
linux\gdk build fixes for EditorClientGdk and FrameGdk

linux\gdk build fixes for EditorClientGdk and FrameGdk

[Krzysztof Kowalczyk]

Created attachment 13047 [details]
linux\gdk build fixes - update bakefiles

linux\gdk build fixes - update bakefiles

[Adam Roben (:aroben)]

Comment on attachment 13047 [details]
linux\gdk build fixes - update bakefiles

         DerivedSources/WebCore/JSHTMLQuoteElement.cpp
+        DerivedSources/WebCore/JSHTMLSelectElement.cpp
         DerivedSources/WebCore/JSHTMLScriptElement.cpp

   Select should go below Script.

   r=me

[Adam Roben (:aroben)]

Comment on attachment 13026 [details]
linux\gdk build fixes for cairo

r=me

[Adam Roben (:aroben)]

Comment on attachment 13022 [details]
Linux\gdk build fixes - el patch numero huit

 #include "ChromeClient.h"
+#include "Shared.h"

   Shared.h shouldn't be necessary anymore.

+    // FIXME: optimize the way CursorQt is optmized: only one copy of a given
+    // cursor type
+    static Cursor c = gdk_cursor_new(GDK_LEFT_PTR);

   I think you've already implemented this FIXME. Since c is static gdk_cursor_new will only be called once.

 #include <assert.h>
 #include <gdk/gdk.h>
+#include "SystemTime.h"

   SystemTime.h should go above <assert.h>

+    m_shiftKey = event->button.state & GDK_SHIFT_MASK != 0;
+    m_ctrlKey = event->button.state & GDK_CONTROL_MASK != 0;
+    m_altKey = event->button.state & GDK_MOD1_MASK != 0;
+    m_metaKey = event->button.state & GDK_MOD2_MASK != 0;

   The '!= 0' is redundant in each of these cases.

   r=me.

[Adam Roben (:aroben)]

Comment on attachment 13046 [details]
linux\gdk build fixes for EditorClientGdk and FrameGdk

+#ifndef EditorClientGtk_H
+#define EditorClientGtk_H

   Sould be EditorClientGdk_h (t -> d and lowercase h)

+#include <gdk/gdk.h>
+#include <gtk/gtk.h>

   These headers should go at the end of the list.

+#include <gdk/gdk.h>
+#include <gtk/gtk.h>

   Ditto (in main.cpp)

   r=me.

[Krzysztof Kowalczyk]

Comment on attachment 13047 [details]
linux\gdk build fixes - update bakefiles

commited. Corrected position of JSHTMLSelectElement.cpp as per review comments.

[Krzysztof Kowalczyk]

Comment on attachment 13026 [details]
linux\gdk build fixes for cairo

commited. Added one last-minute change: a notImplemented() macro for MSVC as well (MSVC doesn't understand __PRETTY_FUNCTION__) since windows port also uses GraphicsContextCairo.cpp

[Krzysztof Kowalczyk]

Comment on attachment 13022 [details]
Linux\gdk build fixes - el patch numero huit

commited. Removed <Shared.h>, re-arranged headers order, removed redundant != 0. FIXME comment refers to the fact that there are several static variables for the same GDK_LEFT_PTR cursor, which could be collapsed into just one, so it's still valid. One last-minute change: adding (notImplemented()) ChromeClientGdk::shouldInterruptJavaScript() (it was added after the patch has been submitted)

[Krzysztof Kowalczyk]

Comment on attachment 13046 [details]
linux\gdk build fixes for EditorClientGdk and FrameGdk

commited. Fix header guard name and re-arranged include order as per review comments.

[Krzysztof Kowalczyk]

Created attachment 13068 [details]
make keyboard link-walking work on gdk

This is the last one in this batch, I promise. Makes gdk's tab key recognized as tab so that keyboard link walking works.

[Krzysztof Kowalczyk]

Created attachment 13074 [details]
fix keyboard link walking; bakefile fix

Make gdk's tab key recognized as tab to fix keyboard link walking. bakefile update for file renaming.

[Darin Adler]

Comment on attachment 13074 [details]
fix keyboard link walking; bakefile fix

r=me