Bug 34571 – [Haiku] Implementation of browser shell

• Component: WebKit API
• Status: RESOLVED INVALID

[Stephan Aßmus]

Created attachment 48128 [details]
Patch implementing HaikuLauncher browser shell.

The patch adds an implementation of a minimal browser shell for the Haiku platform.

[Maxime Simon]

Thanks for the new bug report Stephan.
Though I'm pretty sure that you would have to split this patch into smaller pieces. :-)

[Maxime Simon]

+WebProcess::WebProcess(WebView* webView)
[...]
+    // Default settings - We should have WebViewSettings class for this.
+    WebCore::Settings* settings = m_page->settings();
+    settings->setLoadsImagesAutomatically(true);
+    settings->setMinimumFontSize(5);
+    settings->setMinimumLogicalFontSize(5);
+    settings->setShouldPrintBackgrounds(true);
+    settings->setJavaScriptEnabled(true);
+
+    settings->setDefaultFixedFontSize(14);
+    settings->setDefaultFontSize(14);
+
+    // TODO: Init from system fonts or application settings.
+    settings->setSerifFontFamily("DejaVu Serif");
+    settings->setSansSerifFontFamily("DejaVu Sans");
+    settings->setFixedFontFamily("DejaVu Sans Mono");
+    settings->setStandardFontFamily("DejaVu Serif");
+    settings->setDefaultTextEncodingName("UTF-8");

As said in the TODO comments, we should one day implement a WebSettings class to handle these settings directly from the application (WebBrowser, etc). But that may not be the more urgent thing to do for the moment and we may land this class as it.

[Stephan Aßmus]

All the files depend on each other. Why would it make sense to break the patch into smaller pieces? Of course individual files could be commited which build the foundation for the higher level classes. I just don't understand why it makes a difference.

[Maxime Simon]

(In reply to comment #3)
> All the files depend on each other. Why would it make sense to break the patch
> into smaller pieces? Of course individual files could be commited which build
> the foundation for the higher level classes. I just don't understand why it
> makes a difference.

Indeed you won't be able to build it when only part of them will be landed but as we already have all of them in our computer that doesn't matter. But it's then easier to review and for you to correct what's wrong (you would have to make smaller patches).

[Oliver Hunt]

Comment on attachment 48128 [details]
Patch implementing HaikuLauncher browser shell.


> +enum {
> +    HANDLE_SHUTDOWN                        = 'sdwn',
> +
> +    HANDLE_LOAD_URL                        = 'lurl',
> +    HANDLE_GO_BACK                        = 'back',
> +    HANDLE_GO_FORWARD                    = 'fwrd',
> +
> +    HANDLE_DRAW                            = 'draw',
> +
> +    HANDLE_FRAME_RESIZED                = 'rszd'
> +};

This is incorrect, character literals should be a single character, this triggers warnings in GCC and likely does not do what you are expecting.

Overall i'd also prefer this patch was split into multiple pieces so that it's easier to review.

[Stephan Aßmus]

(In reply to comment #5)
> (From update of attachment 48128 [details])
> 
> > +enum {
> > +    HANDLE_SHUTDOWN                        = 'sdwn',
> > +
> > +    HANDLE_LOAD_URL                        = 'lurl',
> > +    HANDLE_GO_BACK                        = 'back',
> > +    HANDLE_GO_FORWARD                    = 'fwrd',
> > +
> > +    HANDLE_DRAW                            = 'draw',
> > +
> > +    HANDLE_FRAME_RESIZED                = 'rszd'
> > +};
> 
> This is incorrect, character literals should be a single character, this
> triggers warnings in GCC and likely does not do what you are expecting.

These are not character literals, but a 32 bit integer enumeration (on 32 bit host). I can absolutely assure you that this code is correct and does what it is supposed to. I am also not getting GCC warnings. Code like the above is used on BeOS since years to define BMessage "what" constants used in event handler switch blocks. But the code could be rewritten as such:

const uint32 kHandleShutDown = 'sdwn';
const uint32 kHandleLoadURL = 'lurl';
const uint32 kHandleGoBack = 'back';
[...]

That would be equivalent. Would you prefer that?

> Overall i'd also prefer this patch was split into multiple pieces so that it's
> easier to review.

Ok. In which way should I separate the patches? Individual files for each class? Or multiple files grouped by function?

[David Levin]

haiku port has been removed.