Summary: | [Haiku] Implementation of browser shell | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephan Aßmus <superstippi> | ||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED INVALID | ||||||
Severity: | Normal | CC: | levin, oliver, simon.maxime | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | Other | ||||||
Attachments: |
|
Thanks for the new bug report Stephan. Though I'm pretty sure that you would have to split this patch into smaller pieces. :-) +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. 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. (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). 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. (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? haiku port has been removed. |
Created attachment 48128 [details] Patch implementing HaikuLauncher browser shell. The patch adds an implementation of a minimal browser shell for the Haiku platform.