Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New CSS selector ":contains(text)" #42

Merged
merged 26 commits into from
Mar 26, 2018

Conversation

f34nk
Copy link
Contributor

@f34nk f34nk commented Mar 24, 2018

Hey Alex,
I followed your instructions in #36 and added new pseudo class "contains".

  • added "contains" to $func_map = {...}
  • run script perl utils/MyCSS/functions.pl
  • added all "contains" related functions and enums
  • function mycss_selectors_function_begin_contains() in file function.c:
    added selector sub_type MyCSS_SELECTORS_SUB_TYPE_PSEUDO_CLASS_FUNCTION_CONTAINS
  • added test test/myhtml/pseudo_class_contains.c
  • implemented finder function in modest_finder_selector_sub_type_pseudo_class_function_contains() in file pseudo_class.c.

As of now, the selector argument cannot contain single or double quotation marks. That's because I basically duplicated the implementation from pseudo class "has", so behavior and serialization is the same.

Example:

const char *selector = ":contains(Hello)"; // works
const char *selector = ":contains('Hello')"; // Bad Selector
const char *selector = ":contains(\"Hello\")"; // Bad Selector

According to the spec (from 2001) however, quotation marks should also be supported.

The :contains("foo") pseudo-class notation represents an element whose textual contents contain the given substring. The argument of this pseudo-class can be a string (surrounded by double quotes) or a keyword.

So my implementation only solves the "argument as keyword" case.
Tell me what you think.

Best,
Frank

@f34nk f34nk changed the title Add CSS selector for ":contains(text)" New CSS selector ":contains(text)" Mar 24, 2018
@@ -189,9 +190,9 @@ myhtml_tree_node_t * modest_finder_node_combinator_begin(modest_finder_t* finder
if(callback_found)
callback_found(finder, node, selector_list, selector, spec, ctx);
}
else {
else {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra spaces after else {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

// FRANK
// printf("\nmodest_finder_node_combinator_undef()\n");
// printf("\tselector->type = %d\n", (int)selector->type);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to remove comments above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -77,6 +77,62 @@ bool modest_finder_selector_sub_type_pseudo_class_function_has(modest_finder_t*
return false;
}

// concat str1 and str2
char *concat_string(const char *str1, const char *str2)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to core module, append to mycore/utils.c and rename to mycore_str_cat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


bool modest_finder_selector_sub_type_pseudo_class_function_contains(modest_finder_t* finder, myhtml_tree_node_t* base_node, mycss_selectors_entry_t* selector, mycss_selectors_specificity_t* spec)
{
if(base_node){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(base_node == NULL) {
    return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

if(str1) n += strlen(str1);
if(str2) n += strlen(str2);

if((str1 || str2) && (result = malloc(n + 1)) != NULL)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many checks of the same str1 and str2
You can make it easier

if(str1) n += strlen(str1);
if(str2) n += strlen(str2);

if((str1 || str2) && (result = malloc(n + 1)) != NULL)
Copy link
Owner

@lexborisov lexborisov Mar 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

malloc => mycore_malloc and:

result = mycore_malloc(n + 1);
if(result == NULL) {
    return NULL;
}

mycss_selectors_entry_t *next = sel_entry->next;
while(next && !i_found){
if(next->key->data){
data = concat_string(data, " ");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memory leak!
if we allocation memory in concat_string and call concat_string with returned data we get leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course :)

next = next->next;
}
}
if(strstr(text, data) != NULL) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the release (mycore_free(data)) of resources for data?

@@ -34,7 +34,7 @@ const mycss_selectots_function_begin_entry_t * mycss_function_begin_entry_by_nam
% MyCSS_SELECTORS_FUNCTION_NAME_STATIC_SIZE) + 1;

while (mycss_selectors_function_begin_map_index[idx].name)
{
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary spaces after {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


(*new_list)->parent = current_list;

mycss_entry_parser_list_push(entry, mycss_selectors_function_parser_has, entry->parser_switch, entry->parser_ending_token, false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't you implement their parsing function?
Do not use mycss_selectors_function_parser_has in this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

mycore_free(data);
return true;
}
else {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary else

if(strstr(text, data) != NULL) {
    mycore_free(data);
    return true;
}

mycore_free(data);

return false;

mycss_selectors_list_t *list = (mycss_selectors_list_t*)selector->value;
for(size_t i = 0; i < list->entries_list_length; i++) {
char *data = NULL;
data = mycore_malloc(0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(data == NULL) {
    return false;
}

const char *str = sel_entry->key->data;
int length = strlen(str) + 1;
data = mycore_realloc(data, length);
snprintf(&data[0], length, "%s", str);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leak detected if mycore_realloc return NULL. Need check.

if(sel_entry->key->data){
    const char *str = sel_entry->key->data;
    int length = strlen(str) + 1;

    char *new_data = mycore_realloc(data, length);
    if(new_data == NULL) {
        mycore_free(data);
        return false;
    }

    snprintf(&data[0], length, "%s", str);
}

const char *str = next->key->data;
int length = strlen(whitespace) + strlen(str) + 1;
data = mycore_realloc(data, prev + length);
snprintf(&data[prev], length, "%s%s", whitespace, str);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this we have memory leak. see prev comment

const char *str = sel_entry->key->data;
int length = strlen(str) + 1;
data = mycore_realloc(data, length);
if(data == NULL) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leak detected, see comment

mycss_selectors_entry_t *sel_entry = list->entries_list[i].entry;
if(sel_entry->key->data){
const char *str = sel_entry->key->data;
int length = strlen(str) + 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add new line after this

mycore_free(data);
return false;
}
snprintf(&new_data[0], length, "%s", str);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line before

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and change to snprintf(new_data, length, "%s", str);

int prev = strlen(data);
const char *whitespace = (prev > 0) ? " " : "";
const char *str = next->key->data;
int length = strlen(whitespace) + strlen(str) + 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line after

mycore_free(data);
return false;
}
snprintf(&new_data[prev], length, "%s%s", whitespace, str);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line before

@lexborisov
Copy link
Owner

AWESOME!

@lexborisov lexborisov merged commit 87d75c9 into lexborisov:master Mar 26, 2018
@f34nk
Copy link
Contributor Author

f34nk commented Mar 26, 2018

Thanks man! That was fun :)

@lexborisov
Copy link
Owner

You're welcome! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants